[PATCH] D146918: [CMake] Don't set absolute paths as install runpaths on ELF platforms in llvm_setup_rpath()

[Finagolfin via Phabricator via llvm-commits]

finagolfin added a comment.

> To clarify, I meant to ask if it's correct to say "It's incorrect for LLVM_LIBRARY_DIR to be in the install rpath"

I was unaware that `LLVM_LIBRARY_DIR` can be overridden to an installed directory and was only aware of the much more common usecase where it is set to the LLVM build directory by default <https://github.com/llvm/llvm-project/blob/d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6/llvm/CMakeLists.txt#L424>, so my statement should be amended to "It's incorrect for `LLVM_LIBRARY_DIR` to be in the install rpath much of the time," now that you pointed this out.

> it is not expected that the find_package(LLVM) machinery will produce a functioning binary without also setting CMAKE_INSTALL_RPATH to point to the LLVM install directory? Is that right?

I would say that these runpaths have been handled somewhat crudely so far, and favored some uncommon usecases over more common ones. I think that variable presents one way to override that, so I pointed it out.

> I suppose that is what I am questioning, I'd have hoped the build system generator would have made that work.

Given that `LLVM_LIBRARY_DIR` can be either a build directory or an install directory, I don't see how CMake can figure that out automatically: it will require developer input.

> Shouldn't users be setting LLVM_LIBRARY_DIR to their install dir rather than the build dir?

As my link above shows, the default is the build dir.

> Under what circumstances is it desirable to set LLVM_LIBRARY_DIR to the build directory?

Unsure, I'm just dealing with that as it is now.

> If they're setting it to the build directory rather than the install directory, why is it incorrect to refer to the build directory as has been configured, once installed?

Can you rephrase this? I don't know what you mean. The best I can make out is that you mean, "why is it incorrect to add the build directory to the install runpath?" Because as shown in the examples above, the build directory usually doesn't exist in the finally installed system.

> It seems suspicious to me that it has been necessary to diverge behavior across platforms here ("it was said to break the build on macOS and Windows", from the commit message).

That was only for the change in compiler-rt, that change to `llvm_setup_rpath()` was reverted in D148866 <https://reviews.llvm.org/D148866>. As noted in a prior comment on this patch <https://reviews.llvm.org/D146918#4282895>, I coded this very defensively because of the prior failed attempts to remove the compiler-rt runpaths that I also linked, but based on @asb's comment above about seeing this error on linux too, that was almost certainly a mistaken attribution by those devs in the past, at least on Windows. That's why I removed that Windows line later, and had no problem.

I think you are right that this patch broke your usecase, but your usecase is so uncommon, ie building the base LLVM libs and clang separately then installing them to different install prefixes, that we should prioritize the more common ones. However, if the existing CMake overrides don't work well for you, feel free to submit a patch with another toggle that works well for your usecase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146918