[CMake][runtimes] Use variables rather than ":" delimiters

[phosek]

Summary

This is a more idiomatic CMake.

[phosek]

runtimes/CMakeLists.txt

475

There's one other use case of ":" which is used in cache files, I'm not sure if we should replace that one as well?

[phosek]

runtimes/CMakeLists.txt

383–387

I'm wandering whether we even need list of lists here, we may as well just do set(${runtime_name}-${name} ${runtime_name}) and in llvm_ExternalProject_Add just check if the variable is defined, if so use its value otherwise use it as string. What do you think about that approach?

[phosek]

ping

[phosek]

ping again?

[beanz]

Sorry for the long delay on this review. It keeps getting lost in my inbox.

I'm totally fine with the patch as-is, but did have some comments below.

I will defer to your judgement on how you think it is best to proceed because you've been doing a phenomenal job with all this.

runtimes/CMakeLists.txt

383–387

Your suggestion here sounds like it might be simpler. I'd say go with the simpler approach.

475

Ideally I think we probably should replace it. In general I don't like treating : as special characters.

[phosek]

runtimes/CMakeLists.txt

475

I'll do this in a separate patch since it'll require more changes.

[phosek]

runtimes/CMakeLists.txt

475

Done in D48061.

cmake/modules/LLVMExternalProjectUtils.cmake

if(NOT ARG_NO_INSTALL)
 
     add_llvm_install_targets(install-${name}
                              DEPENDS ${name}
                              COMPONENT ${name})
   endif()
 
   # Add top-level targets
   foreach(target ${ARG_EXTRA_TARGETS})
-    string(REPLACE ":" ";" target_list ${target})
-    list(GET target_list 0 target)
-    list(LENGTH target_list target_list_len)
-    if(${target_list_len} GREATER 1)
-      list(GET target_list 1 target_name)
+    if(DEFINED ${target})
+      set(target_name "${${target}}")
     else()
       set(target_name "${target}")
     endif()
-    llvm_ExternalProject_BuildCmd(build_runtime_cmd ${target} ${BINARY_DIR})
-    add_custom_target(${target_name}
+    llvm_ExternalProject_BuildCmd(build_runtime_cmd ${target_name} ${BINARY_DIR})
+    add_custom_target(${target}
       COMMAND ${build_runtime_cmd}
       DEPENDS ${name}-configure
       WORKING_DIRECTORY ${BINARY_DIR}
       VERBATIM
       USES_TERMINAL)
   endforeach()
 endfunction()

runtimes/CMakeLists.txt

function(runtime_register_target name target)
     include(${LLVM_BINARY_DIR}/runtimes/${name}/Components.cmake OPTIONAL)
     set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${LLVM_BINARY_DIR}/runtimes/${name}/Components.cmake)
 
     set(${name}_deps ${ARG_DEPS})
     if(NOT name STREQUAL target)
       list(APPEND ${name}_deps runtimes-${target})
     endif()
 
     foreach(runtime_name ${runtime_names})
-      list(APPEND ${name}_extra_targets
-        "${runtime_name}:${runtime_name}-${name}"
-        "install-${runtime_name}:install-${runtime_name}-${name}"
-        "install-${runtime_name}-stripped:install-${runtime_name}-${name}-stripped")
+      set(${runtime_name}-${name} ${runtime_name})
+      set(install-${runtime_name}-${name} install-${runtime_name})
+      set(install-${runtime_name}-${name}-stripped install-${runtime_name}-stripped)
+      list(APPEND ${name}_extra_targets ${runtime_name}-${name} install-${runtime_name}-${name} install-${runtime_name}-${name}-stripped)
       if(LLVM_INCLUDE_TESTS)
-        list(APPEND ${name}_test_targets "check-${runtime_name}:check-${runtime_name}-${name}")
+        set(check-${runtime_name}-${name} check-${runtime_name} )
+        list(APPEND ${name}_test_targets check-${runtime_name}-${name})
       endif()
     endforeach()
 
     foreach(target_name IN LISTS SUB_COMPONENTS SUB_INSTALL_TARGETS)
-      list(APPEND ${name}_extra_targets "${target_name}:${target_name}-${name}")
+      set(${target_name}-${name} ${target_name})
+      list(APPEND ${name}_extra_targets ${target_name}-${name})
     endforeach()
 
     if(LLVM_INCLUDE_TESTS)
-      list(APPEND ${name}_test_targets
-        "runtimes-test-depends:runtimes-test-depends-${name}"
-        "check-runtimes:check-runtimes-${name}")
+      set(runtimes-test-depends-${name} runtimes-test-depends)
+      set(check-runtimes-${name} check-runtimes)
+      list(APPEND ${name}_test_targets runtimes-test-depends-${name} check-runtimes-${name})
       foreach(target_name IN LISTS SUB_CHECK_TARGETS)
-        list(APPEND ${name}_test_targets "${target_name}:${target_name}-${name}")
-        list(APPEND test_targets "${target_name}-${name}")
+        set(${target_name}-${name} ${target_name})
+        list(APPEND ${name}_test_targets ${target_name}-${name})
+        list(APPEND test_targets ${target_name}-${name})
       endforeach()
       set(test_targets "${test_targets}" PARENT_SCOPE)
     endif()
 
     get_cmake_property(variableNames VARIABLES)
     foreach(variableName ${variableNames})
       if(variableName MATCHES "^RUNTIMES_${name}")
         string(REPLACE "RUNTIMES_${name}_" "" new_name ${variableName})
else()
           add_custom_target(check-runtimes)
           add_custom_target(runtimes-test-depends)
           set(test_targets "")
         endif()
       endif()
 
       foreach(name ${LLVM_RUNTIME_TARGETS})
         set(target ${name})
         string(REPLACE ":" ";" target_list ${target})
         list(GET target_list 0 name)
         list(LENGTH target_list target_list_len)
         if(${target_list_len} GREATER 1)
           list(GET target_list 1 target)
         endif()
 
         runtime_register_target(${name} ${target}
           DEPS ${deps}