Page MenuHomePhabricator

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

Authored by tambre on Dec 13 2020, 2:33 AM.

Details

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.

Diff Detail

Event Timeline

tambre created this revision.Dec 13 2020, 2:33 AM
tambre requested review of this revision.Dec 13 2020, 2:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2020, 2:33 AM
tambre updated this revision to Diff 311435.Dec 13 2020, 2:35 AM

Fix commit message

tambre retitled this revision from [CMake] Fix BUILD_WITH_INSTALL_RPATH being set when LLVM_LOCAL_RPATH is empty to [CMake] Fix BUILD_WITH_INSTALL_RPATH being set when using custom build rpath.Dec 13 2020, 2:35 AM
tambre retitled this revision from [CMake] Fix BUILD_WITH_INSTALL_RPATH being set when using custom build rpath to [CMake] Don't enable BUILD_WITH_INSTALL_RPATH when using custom build rpath.Dec 13 2020, 3:45 AM
tambre updated this revision to Diff 311437.Dec 13 2020, 3:51 AM

Fix all occurrences. Keep INSTALL_RPATH and only disable BUILD_WITH_INSTALL_RPATH.

tambre edited the summary of this revision. (Show Details)Dec 13 2020, 3:55 AM
tambre edited the summary of this revision. (Show Details)Dec 13 2020, 4:00 AM
phosek added a comment.Jan 6 2021, 1:38 AM

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 updated this revision to Diff 315075.Jan 7 2021, 2:38 AM

Avoid extra variable

tambre marked an inline comment as done.Jan 7 2021, 2:47 AM

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 accepted this revision.Jan 7 2021, 2:11 PM

LGTM

This revision is now accepted and ready to land.Jan 7 2021, 2:11 PM
This revision was landed with ongoing or failed builds.Jan 7 2021, 10:31 PM
This revision was automatically updated to reflect the committed changes.
tambre marked an inline comment as done.

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 added a subscriber: sbc100.Jan 8 2021, 7:08 AM

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 added a comment.Jan 8 2021, 7:11 AM

OK, -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON worked for me

tambre added a comment.EditedJan 8 2021, 10:25 AM

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.

dmajor added a subscriber: dmajor.Jan 8 2021, 10:54 AM

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!