search

ros / class_loader

ROS-independent library for dynamic class (i.e. plugin) introspection and loading from runtime libraries
http://www.ros.org/wiki/class_loader
35 stars 95 forks source link

Resolve symlinks fully before loading the library. #207

Closed iwanders closed 1 year ago

iwanders commented 1 year ago

Relates to https://github.com/ros/ros/pull/309.

Our Nix-based ros builds create a directory that has each ros package symlinked, so instead of one directory holding all our ros packages, we have one directory holding symlinks to the ros packages. The package move_base would exist in /nix/store/<hash>-sdk/merged/move_base/move_base, but /nix/store/<hash>-sdk/merged/move_base/ would be a symlink to /nix/store/<hash>-move_base/.

For plugins, they could be found in /nix/store/<hash>-sdk/merged/my_package/libmy_plugin.so, but /nix/store/<hash>-sdk/merged/my_package/ would be a symlink to /nix/store/<hash>-my_package/.

Without resolving symlinks, core dumps would contain library paths to the full /nix/store/<hash>-sdk/merged/my_package/libmy_plugin.so path, which requires developers to download that completely. If we resolve the symlinks just before loading the plugin developers only need /nix/store/<hash>-my_package/ at that particular version.

fyi @mikepurvis , @jasonimercer

gbiggs commented 1 year ago

Can you please re-target this PR to the rolling branch? We can backport it once it's merged.

iwanders commented 1 year ago

Can you please re-target this PR to the rolling branch? We can backport it once it's merged.

Yep, I'll do that, I see just changing the branch won't do it, let me branch off from rolling and cherrypick this.

iwanders commented 1 year ago

Cherrypicking the commit into rolling is non trivial; there's no use of boost in the rolling branch? And it seems to be targetting c++14 which doesn't have std::filesystem either.

Not sure how you would like to see those problems resolved; regardless it's not something trivial I can do without having access to my work laptop, so won't be today for sure.

gbiggs commented 1 year ago

Let's target it to noetic-devel instead then.

iwanders commented 1 year ago

Ok, this should be it, I confused melodic with noetic :grimacing:

iwanders commented 1 year ago

I hope this pipeline reruns when the target branch is changed, because that dco check currently mentions commits that are not part of the delta.

gbiggs commented 1 year ago

Yes, those appear to be unrelated.

gbiggs commented 1 year ago

Tested locally and it works, so merging.

iwanders commented 1 year ago

Awesome, thanks for the quick turnaround @gbiggs.