[CMake] Don't enable BUILD_WITH_INSTALL_RPATH when using custom build rpath

[tambre]

Summary

When BUILD_WITH_INSTALL_RPATH is enabled it prevents using a custom rpath only
for the build tree as the install rpath will be used. This makes it impossible to run a
runtimes build when compiling with Clang and wanting the installed rpath to be
empty (i.e. -DCMAKE_BUILD_RPATH="<some path>" -DCMAKE_SKIP_INSTALL_RPATH=ON).

Disable BUILD_WITH_INSTALL_RPATH when CMAKE_BUILD_RPATH is non-empty to
allow for such build scenarios.

[tambre]

Fix commit message

[tambre]

Fix all occurrences. Keep INSTALL_RPATH and only disable BUILD_WITH_INSTALL_RPATH.

[tambre]

ping

[tambre]

ping

[phosek]

This is fine with me, but I wonder we should avoid setting this property for every target and instead rely on CMAKE_BUILD_WITH_INSTALL_RPATH (we could set CMAKE_BUILD_WITH_INSTALL_RPATH to ON if unset to preserve the current behavior).

llvm/cmake/modules/AddLLVM.cmake

869–877

You could also consider something like this to avoid the extra variable (the same below).

[tambre]

Avoid extra variable

[tambre]

In D93177#2481569, @phosek wrote:

This is fine with me, but I wonder we should avoid setting this property for every target and instead rely on CMAKE_BUILD_WITH_INSTALL_RPATH (we could set CMAKE_BUILD_WITH_INSTALL_RPATH to ON if unset to preserve the current behavior).

That would be better.
Since there's a small chance some places are relying on it being OFF globally, I'd prefer to do that as follow-up, so it can be reverted more easily.

llvm/cmake/modules/AddLLVM.cmake

869–877

Thanks!

[phosek]

LGTM

[hubert.reinterpretcast]

We're seeing:

CMake Error at cmake/modules/AddLLVM.cmake:861 (add_executable):
  The install of the verify-uselistorder target requires changing an RPATH
  from the build tree, but this is not supported with the Ninja generator
  unless on an ELF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH
  variable may be set to avoid this relinking step.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:1209 (add_llvm_executable)
  tools/verify-uselistorder/CMakeLists.txt:10 (add_llvm_tool)

Is it the general belief that the status quo before this patch is equivalent to setting CMAKE_BUILD_WITH_INSTALL_RPATH to ON?

[sbc100]

This change broke the emscripten SDK builders which build with LLVM_LINK_LLVM_DYLIB and expect the resulting binaries to be portable/relocatable.

Before (portable):

readelf -d ../llvm-build/bin/llc | grep runpath
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib]

After (non-portable):

readelf -d ../llvm-build/bin/llc | grep runpath
 0x000000000000001d (RUNPATH)            Library runpath: [/usr/local/google/home/sbc/dev/wasm/llvm-build/lib]

It looks like this was probably intentional and we probably just need to set some new cmake option? We don't set CMAKE_BUILD_RPATH but we do use LLVM_LINK_LLVM_DYLIB and LLVM_BUILD_LLVM_DYLIB: https://github.com/WebAssembly/waterfall/blob/2291ee46c5a5d04cf1269747d6193e530217e02c/src/build.py#L887

I haven't really dug into the details but I tried setting -DCMAKE_SKIP_INSTALL_RPATH=ON and -DCMAKE_SKIP_INSTALL_RPATH=OFF to no avail. What should I be going if I want the old/portable behaviour of $ORIGIN/../lib?

[sbc100]

OK, -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON worked for me

[tambre]

Sorry for the breakage.
I reverted (D94319) and now re-landed (D94322). Seems the check for empty CMAKE_BUILD_RPATH got incorrectly reverted when I addressed the review comment.

[hubert.reinterpretcast]

In D93177#2487304, @tambre wrote:

I reverted (D94319) and now re-landed (D94322). Seems the check for empty CMAKE_BUILD_RPATH got incorrectly reverted when I addressed the review comment.

Thanks for the fix!

llvm/cmake/modules/AddLLVM.cmake

macro(add_llvm_executable name)
   else()
     add_executable(${name} ${ALL_FILES})
   endif()
 
   setup_dependency_debugging(${name} ${LLVM_COMMON_DEPENDS})
 
   if(NOT ARG_NO_INSTALL_RPATH)
     llvm_setup_rpath(${name})
-  elseif (LLVM_LOCAL_RPATH)
-    set_target_properties(${name} PROPERTIES
-                          BUILD_WITH_INSTALL_RPATH On
-                          INSTALL_RPATH "${LLVM_LOCAL_RPATH}")
+  else()
+    # Enable BUILD_WITH_INSTALL_RPATH unless CMAKE_BUILD_RPATH is set.
+    if(NOT "${CMAKE_BUILD_RPATH}" STREQUAL "")
+      set_property(TARGET ${name} PROPERTY BUILD_WITH_INSTALL_RPATH ON)
+    endif()
+
+    set_property(TARGET ${name} PROPERTY INSTALL_RPATH "${LLVM_LOCAL_RPATH}")
   endif()
 
   if(DEFINED windows_resource_file)
     set_windows_version_resource_properties(${name} ${windows_resource_file})
   endif()
 
   # $<TARGET_OBJECTS> doesn't require compile flags.
   if(NOT LLVM_ENABLE_OBJLIB)
     llvm_update_compile_flags(${name})
   endif()
 
if(LLVM_LINKER_IS_GNULD)
       # $ORIGIN is not interpreted at link time by ld.bfd
       set_property(TARGET ${name} APPEND_STRING PROPERTY
                    LINK_FLAGS " -Wl,-rpath-link,${LLVM_LIBRARY_OUTPUT_INTDIR} ")
     endif()
   else()
     return()
   endif()
 
+  # Enable BUILD_WITH_INSTALL_RPATH unless CMAKE_BUILD_RPATH is set.
+  if(NOT "${CMAKE_BUILD_RPATH}" STREQUAL "")
+    set_property(TARGET ${name} PROPERTY BUILD_WITH_INSTALL_RPATH ON)
+  endif()
+
   set_target_properties(${name} PROPERTIES
-                        BUILD_WITH_INSTALL_RPATH On
                         INSTALL_RPATH "${_install_rpath}"
                         ${_install_name_dir})
 endfunction()
 
 function(setup_dependency_debugging name)
   if(NOT LLVM_DEPENDENCY_DEBUGGING)
     return()
   endif()