Fix ament cmake transitive dependency issue #293
Conversation
|
@jwnimmer-tri Hey Jeremy would you mind taking a look at this PR? |
|
+@IanTheEngineer do you know any https://github.com/RobotLocomotion/drake-ros member who would like to review this? To me, the ament part looks okay, but manually setting LD_LIBRARY_PATH is usually an obscenity and should be avoided with prejudice. Hopefully there is another way to get it working. I imagine the ament ecosystem already has some better way to deal with library paths? |
|
@IanTheEngineer last call to find someone to support this ament example. If we can't find anyone to support it, then I will merge this PR as-is and follow up by documenting the ament example as outside of our support footprint, i.e., "community maintained - PRs welcome" and remove it from CI. |
|
I should probably ping @EricCousineau-TRI also. |
There was a problem hiding this comment.
Thanks, Jeremy. My apologies on the delayed response to you and @weithegreat. I did not have my filters appropriately configured to realize that you tagged me in this repository.
I agree with the ament_export_dependencies change, but do not understand manually configuring the LD_LIBRARY_PATH. That doesn't seem necessary, but I would be interested to find out more.
Regardless, I think this drake_ament_cmake_installed example could stay outside our support footprint, as it's not something that we regularly consume in drake-ros or otherwise.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee iantheengineer, platform LGTM missing (waiting on @weithegreat)
drake_ament_cmake_installed/README.md line 18 at r1 (raw file):
Add drake library path to Linux search path.echo "export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/opt/drake/lib" >> ~/.bashrc
Does your environment work without adding this LD_LIBRARY_PATH export and only the ament_export_dependencies(drake) change below?
Depending on the reader's environment setup, this export could be problematic, particularly if multiple versions of Drake are added.
There was a problem hiding this comment.
+@IanTheEngineer for feature review, please.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Dismissed @IanTheEngineer from a discussion.
Reviewable status: all discussions resolved, LGTM missing from assignee iantheengineer, platform LGTM from [jwnimmer-tri] (waiting on @weithegreat)
drake_ament_cmake_installed/README.md line 18 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
Does your environment work without adding this
LD_LIBRARY_PATHexport and only theament_export_dependencies(drake)change below?Depending on the reader's environment setup, this export could be problematic, particularly if multiple versions of Drake are added.
Not hearing anything, we might as well just merge the CMake fix and leave this part for another day. I reverted this part.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all discussions resolved, platform LGTM from [jwnimmer-tri] (waiting on @weithegreat)
The current amend cmake example build without problem, however there will be issue finding drake dependency for libraries depends on such a library
Closes #292.
This change is