-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search RUNPATH to find library on Linux #44
Closed
Closed
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
3b8cf64
On Linux, search RUNPATH in addition to LD_LIBRARY_PATH
wieset 3934468
Add test for searching RUNPATH to find library on Linux
wieset f1d15bb
Make sure RUNPATH is set correctly
wieset d4542a3
Fix header order
wieset 2b7b0b9
Fix pointer code style
wieset 71b46d1
Put ifdef around include
wieset 0d49a40
Extract RUNPATH logic into helper method
wieset f4ee238
Return empty list in RUNPATH search on win / macos
wieset c12ae09
Set library path env var through cmake test properties and reuse find…
wieset f691a10
Add const qualifier
wieset eaad9e4
Merge branch 'master' into search-runpath
wieset File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall thing that concerns me about this is this bit. As far as I know, we don't currently set RPATH/RUNPATH on any of our shared objects. In order for this to truly work in general, we would have to start setting that, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to reply. I think you are right, for this to work in general, you would need to have
set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
for all projects.What we are currently doing in our project is using
set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
together withset(CMAKE_EXE_LINKER_FLAGS "-Wl,--disable-new-dtags")
. This makes use of RPATH instead of RUNPATH. RPATH works with indirect shared library dependencies, whereas RUNPATH does not. This, together with the changes from this pull request, allows us to load all shared libraries without having to rely onLD_LIBRARY_PATH
. However, RPATH is deprecated, so I don't think this is a good general solution.Would it be an option to
set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
globally for ament_cmake? I am not sure about any downsides of this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been no activity here in quite a while, so I just wanted to ask if there is any update on this. As a workaround, we are currently running our node in a root shell, but of course we would prefer not to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wanted to check on the status of this. I'm working on a linuxdeploy plugin for building AppImages that bundle a ROS2 app and its dependencies (similar to the linuxdeploy qt plugin).
Certain dependencies, like RMW implementations, are searched for at runtime with RPATH/RUNPATH excluded, so even if the executable's RPATH points to the place where these libraries are located, find_library_path() fails to pick them up. Would be nice to not have to rely on setting environment variables like LD_LIBRARY_PATH or having to source the setup.bash scripts as a custom init step in the AppImage's entrypoint (they are already slow enough to start, due to the squashfs mounting step 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think there are two different things going on in this PR (please correct me if I'm wrong):
find_library.cpp
).I'm fine with the first one; in our current system, that will really have no effect, but gets us closer to what @wieset and others want. I'm much less sure about the implications about the second one, which I think is what is mostly holding this up.
So my suggestion here is that we split out the second part from this PR, and just concentrate on getting the "search in RPATH" changes in. Once that is in, we can consider the "force RUNPATH -> RPATH" separately. Does that make sense?
(also, this PR needs a rebase since things have changed in the meantime)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, there are two things going on. I think the first one is obsolete now with ros2/rcutils#320 which is great! The second one is actually a bit different. It does not force
RPATH
instead ofRUNPATH
at all. Even though CMake usesRPATH
in their variable names, it refers toRUNPATH
sinceRPATH
is deprecated.BUILD_RPATH ${CMAKE_CURRENT_BINARY_DIR}
is necessary so thatRUNPATH
in the build binary gets populated with the path to the test libraryINSTALL_RPATH_USE_LINK_PATH TRUE
is necessary so thatRUNPATH
in the installed binary gets populated properlyHowever, thinking about it again,
INSTALL_RPATH_USE_LINK_PATH TRUE
needs to be set for ALL packages so thatRUNPATH
can be used as an alternative toLD_LIBRARY_PATH
. The reason for this is thatRUNPATH
only works for direct shared object dependencies. See also the discussion here: https://bugs.launchpad.net/ubuntu/+source/glibc/+bug/1737608And this is what's necessary for us to be able to run a node with
setcap
since it dropsLD_LIBRARY_PATH
. A solution I see would be to haveset(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)
in theament_package()
macro.I hope this explanation is clear. What do you think about closing this PR and continuing the discussion in https://github.com/ament/ament_cmake/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable to me, thanks. I'll go ahead and close this; please feel free to open an issue over there.