Skip to content

Use CMake's internal RPATH setting#13

Merged
josephbirkner merged 1 commit intoKlebert-Engineering:mainfrom
Stat1cV01D:main
Mar 28, 2024
Merged

Use CMake's internal RPATH setting#13
josephbirkner merged 1 commit intoKlebert-Engineering:mainfrom
Stat1cV01D:main

Conversation

@Stat1cV01D
Copy link
Contributor

Usually when a binary is built CMake links external libraries with a hardcoded path until cmake install is called. At that point CMake switches RPATH to be more generic. You may read more at the dedicated help page.
Let me know if I missed any detail.

Cheers!

@fklebert fklebert requested a review from josephbirkner March 15, 2024 08:16
@josephbirkner
Copy link
Collaborator

Hey @Stat1cV01D, thanks! I generally like this approach much more than the Python loop. BUT, what is important to consider: The Python loop changes the RPATH not just for the main wheel library, but also for each packaged dependency.so. So I would propose to keep your cmake change, but also keep the patchelf call in Python.

@Stat1cV01D
Copy link
Contributor Author

Stat1cV01D commented Mar 21, 2024

I see your reason, but I am not sure it is the approach that is usually taken with CMake-based projects. If I use dependent libraries in a wheel, I'd expect them to either be ready for end users (with proper RPATHs) or in a neighboring CMake project that also follows the similar RPATH strategy.

One other way I didn't explore (as it would require more changes) is to make a wheel out of "cmake install"-ed binaries (so not a POST_BUILD command, but more of a install(CODE...)1). Would this be a better solution or just a sidestep?

Another point for me personally was to remove the dependency on patchelf as I couldn't install it in a constrained environment (not a sudoer).

Footnotes

  1. https://cmake.org/cmake/help/latest/command/install.html#code

@josephbirkner
Copy link
Collaborator

@Stat1cV01D Thanks for the explanation. I agree. Will merge this 👍

@josephbirkner josephbirkner merged commit 611ead9 into Klebert-Engineering:main Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants