find_library: Prefer currently loaded libraries over searching `LD_LIBRARY_PATH` about rcutils

[hidmic]

Hmm, I'm starting to wonder why we need rcpputils::find_library_path() to begin with. If we simply provide rcutils_load_shared_library() or its rcpputils::SharedLibrary wrapper with a relative path, dlopen and LoadLibrary will search paths and yield loaded object files. Documentation for both suggests that already loaded object files will not be pulled in again. By doing this, RPATHs, LD_LIBRARY_PATHs, RUNPATHs, PATHs, and preloaded libraries would be honored by default.

CC @EricCousineau-TRI @wieset @clalancette.

[EricCousineau-TRI]

@ivanpauno Probably not anytime in the near future (~6mo to 1yr), unfortunately :(

[wieset]

Created a pull request at ros2/rcpputils#44

[hidmic]

Indeed we can. Thanks for the bump!

[EricCousineau-TRI]

Here is a working prototype on Linux:
EricCousineau-TRI/rcpputils@b66a3d9

Cribbing from Drake source:
https://github.com/RobotLocomotion/drake/blob/4c6246197/common/find_loaded_library.cc

[EricCousineau-TRI]

At this commit for this Bazel repro project, I can now either specify the RMW implementation at linking time, or defer to environment variables:
EricCousineau-TRI/repro@cea1102

@dirk-thomas I would find this extremely useful. Can I ask what (if any) evidence might sway you to consider this? :D

[dirk-thomas]

Can I ask what (if any) evidence might sway you to consider this?

Since the goal of having a distro is to ensure stability backports are for bug fixes only. Enhancements should target the next distro.

The first test packages of Dashing are targeted to be available in the first week of April.

[EricCousineau-TRI]

Aye, sounds good. Will await the merge of ros2/rcpputils#3, and then file this against the dev branch. Thanks!

[wieset]

We have a ros2 node that needs capabilities set via setcap which leads to LD_LIBRARY_PATH being omitted during execution. So we are setting RPATH in the binary to find shared libraries. However, as find_library_path relies on LD_LIBRARY_PATH the node fails with

terminate called after throwing an instance of 'rclcpp::exceptions::RCLError'
  what():  failed to initialized rcl init options: failed to find shared library of rmw implementation. Searched rmw_fastrtps_cpp, at /tmp/binarydeb/ros-eloquent-rmw-implementation-0.8.2/src/functions.cpp:130, at /tmp/binarydeb/ros-eloquent-rcl-0.8.3/src/rcl/init_options.c:55

If I understand correctly, the proposal by @EricCousineau-TRI would solve this problem. Any chance to get this functionality in the next release? Or any suggestions for a workaround?

[ivanpauno]

@EricCousineau-TRI are you planning to iterate on this?

[dirk-thomas]

If I understand correctly, the proposal by @EricCousineau-TRI would solve this problem.

@wieset How would it solve the problem if the library you are trying to find / load isn't already loaded?

[wieset]

@dirk-thomas I looked at @EricCousineau-TRI's example code and you are right, it would not solve my problem if the library isn't already loaded. I guess I could link it at build time so that it gets loaded at program startup?

However, to preserve loading at run time, I see two options: Either RPATH would have to be searched, which I can set through CMAKE_INSTALL_RPATH, or another environment variable could be used instead of LD_LIBRARY_PATH, e.g. RMW_LIBRARY_PATH. I mentioned both those options in ros2/rcpputils#40.

[dirk-thomas]

@wieset Customizing your build to use rpath for this use case sounds like the way to go.

[wieset]

@dirk-thomas Agreed, and I am already using RPATH to load all other libraries. But that doesn't get me around the fact that find_library_path() for the rmw library currently doesn't look there. Or am I missing something?

[dirk-thomas]

But that doesn't get me around the fact that find_library_path() for the rtw library currently doesn't look there.

You are right. I missed the context on this ticket.

I wonder if it makes sense to introduce a separate environment variable for this or if you should just make sure when running executables as root to set the existing environment variables yourself explicitly.

[wieset]

As far as I understand, LD_LIBRARY_PATH is ignored completely for setcap / setuid executables, so setting it myself wouldn't help unfortunately. See http://man7.org/linux/man-pages/man8/ld.so.8.html

[dirk-thomas]

Not sure if introducing a new env var like ROS_LIBRARY_PATH to bypass that security limitation is a good idea. ld only wouldn't filter that because it doesn't know about it and it ignores LD_LIBRARY_PATH due to security considerations.

Please feel free to propose PRs to add this enhancement (as indicated by the "help wanted" label).

[wieset]

Will look into into it!

[EricCousineau-TRI]

FWIW @hidmic, @IanTheEngineer, and I are starting to discuss this with other folk at TRI. First towards our internal uses, then towards integration with things like Drake.

[hidmic]

Alright, see #320 and connected PRs for a first stab at this. @wieset this should (indirectly) solve your issues as well. These changes render rcpputils::find_library_path obsolete though (for usage in core packages).

[clalancette]

@hidmic Now that we've landed #320 and friends, can we close this out?

[wieset]

Thanks a lot @hidmic, I think this gets me halfway there. RUNPATH still needs to get populated properly. I'll continue the discussion with @clalancette in ros2/rcpputils#44.