[PATCH] D127800: [llvm-driver] Generate symlinks instead of executables for tools

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: clang/cmake/modules/AddClang.cmake:182
+  get_property(LLVM_DRIVER_TOOLS GLOBAL PROPERTY LLVM_DRIVER_TOOLS)
+  if(${dest} IN_LIST LLVM_DRIVER_TOOLS)
+    set_property(GLOBAL APPEND PROPERTY LLVM_DRIVER_TOOL_SYMLINKS ${name})
----------------
Should we check `LLVM_TOOL_LLVM_DRIVER_BUILD` here as well?


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:866
 
-macro(add_llvm_executable name)
+macro(setup_llvm_executable name)
   cmake_parse_arguments(ARG
----------------
This name isn't particularly descriptive, how about `setup_llvm_driver`?


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:868-870
     "DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH;SUPPORT_PLUGINS;GENERATE_DRIVER"
     "ENTITLEMENTS;BUNDLE_PATH"
     "DEPENDS"
----------------
Most of these aren't being used in this function, could we prune this list and only keep the ones that are needed here?


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:916
 
+macro(add_llvm_executable name)
+  setup_llvm_executable(${name} ${ARGN})
----------------
Conversely, I think the arguments that are only used in this macro should be parsed here with `cmake_parse_arguments`.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:2013
+  get_property(LLVM_DRIVER_TOOLS GLOBAL PROPERTY LLVM_DRIVER_TOOLS)
+  if(${dest} IN_LIST LLVM_DRIVER_TOOLS)
+    set_property(GLOBAL APPEND PROPERTY LLVM_DRIVER_TOOL_SYMLINKS ${name})
----------------
Should we check `LLVM_TOOL_LLVM_DRIVER_BUILD `here as well?


================
Comment at: llvm/tools/CMakeLists.txt:67
+  # it can't add_custom_command that happens after llvm-driver is built because
+  # llvm-driver was not created in that directory so it must be placed here.
+  generate_driver_tool_targets()
----------------
Nit: This is superfluous.


================
Comment at: llvm/tools/llvm-driver/CMakeLists.txt:34
+  foreach(name IN LISTS LLVM_DRIVER_TOOLS LLVM_DRIVER_TOOL_SYMLINKS)
+    add_llvm_tool_symlink(${name} llvm-driver ALWAYS_GENERATE)
+    # Always generate install targets
----------------
The issue you described with subdirectory is a known limitation of CMake when using post build commands.

Do you need `ALWAYS_GENERATE` here? Have you tried it without that option. That would remove the limitation and I //think// it should be sufficient.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127800/new/

https://reviews.llvm.org/D127800