[CMake] Support symlinks even with LLVM_INSTALL_TOOLCHAIN_ONLY

[phosek]

Summary

When LLVM_INSTALL_TOOLCHAIN_ONLY is used and LLVM_TOOLCHAIN_TOOLS contains a tool which is a symlink, it would be ignored. This already worked before but got broken in r282510.

[beanz]

Looking at this, I messed up that logic worse than this fixes. I completely broke LLVM_INSTALL_TOOLCHAIN_ONLY=Off for symlinks.

I think something like this would cover your case and fix LLVM_INSTALL_TOOLCHAIN_ONLY:

diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake
index 51ae62d..00f2e80 100644
--- a/cmake/modules/AddLLVM.cmake
+++ b/cmake/modules/AddLLVM.cmake
@@ -1264,11 +1264,11 @@ function(add_llvm_tool_symlink name dest)
     set_target_properties(${target_name} PROPERTIES FOLDER Tools)
 
     # Make sure both the link and target are toolchain tools
-    if (NOT ${name} IN_LIST LLVM_TOOLCHAIN_TOOLS OR NOT ${dest} IN_LIST LLVM_TOOLCHAIN_TOOLS)
-      return()
+    if (${name} IN_LIST LLVM_TOOLCHAIN_TOOLS OR ${dest} IN_LIST LLVM_TOOLCHAIN_TOOLS)
+      set(TOOL_IS_TOOLCHAIN On)
     endif()
 
-    if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY AND LLVM_BUILD_TOOLS )
+    if ((TOOL_IS_TOOLCHAIN OR NOT LLVM_INSTALL_TOOLCHAIN_ONLY) AND LLVM_BUILD_TOOLS)
       llvm_install_symlink(${name} ${dest})
     endif()
   endif()

Thoughts?

[phosek]

LGTM! Shouldn't TOOL_IS_TOOLCHAIN be lowercase since a lower-case variable (I've seen both variants used in LLVM so I'm not sure if there is convention)? Shall I just update the diff with your version or do you want submit a new diff?

[phosek]

I just tested this with our toolchain build and seems to be working fine.

[beanz]

One comment below. Otherwise LGTM, go ahead and commit.

Thanks for catching this!

cmake/modules/AddLLVM.cmake

1267 ↗

(On Diff #72982)

This should actually be AND not OR since we shouldn't install the symlink without also installing the tool.

llvm/trunk/cmake/modules/AddLLVM.cmake

function(add_llvm_tool_symlink name dest)
   else()
     add_custom_command(OUTPUT ${output_path}
                      COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${dest_binary}" "${output_path}"
                      DEPENDS ${dest})
     add_custom_target(${target_name} ALL DEPENDS ${output_path})
     set_target_properties(${target_name} PROPERTIES FOLDER Tools)
 
     # Make sure both the link and target are toolchain tools
-    if (NOT ${name} IN_LIST LLVM_TOOLCHAIN_TOOLS OR NOT ${dest} IN_LIST LLVM_TOOLCHAIN_TOOLS)
-      return()
+    if (${name} IN_LIST LLVM_TOOLCHAIN_TOOLS AND ${dest} IN_LIST LLVM_TOOLCHAIN_TOOLS)
+      set(TOOL_IS_TOOLCHAIN ON)
     endif()
 
-    if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY AND LLVM_BUILD_TOOLS )
+    if ((TOOL_IS_TOOLCHAIN OR NOT LLVM_INSTALL_TOOLCHAIN_ONLY) AND LLVM_BUILD_TOOLS)
       llvm_install_symlink(${name} ${dest})
     endif()
   endif()
 endfunction()
 
 function(llvm_externalize_debuginfo name)
   if(NOT LLVM_EXTERNALIZE_DEBUGINFO)
     return()