Skip to content
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

[ros2] Extend plugin path instead of overwriting it in ign_gazebo launch script #122

Merged
merged 1 commit into from
Dec 4, 2020
Merged

[ros2] Extend plugin path instead of overwriting it in ign_gazebo launch script #122

merged 1 commit into from
Dec 4, 2020

Conversation

AndrejOrsula
Copy link
Contributor

IGN_GAZEBO_SYSTEM_PLUGIN_PATH is currently being overwritten by LD_LIBRARY_PATH when using ign_gazebo.launch.py. I am failing to find a mention of LD_LIBRARY_PATH with respect to plugins in Finding resources, so I am not sure if it is meant to be an alternative.

Unless I am missing something, I believe that IGN_GAZEBO_SYSTEM_PLUGIN_PATH should be treated the same way in both of these cases for worlds with custom plugins.

  • ign gazebo example.sdf
  • ros2 launch ros_ign_gazebo ign_gazebo.launch.py ign_args:="example.sdf"

To keep this change backwards compatible for people who export LD_LIBRARY_PATH to find custom plugins, I suggest extending IGN_GAZEBO_SYSTEM_PLUGIN_PATH with LD_LIBRARY_PATH instead of overwriting it.

- IGN_GAZEBO_SYSTEM_PLUGIN_PATH was overwritten by LD_LIBRARY_PATH
- Now it is instead extended by LD_LIBRARY_PATH
- This allows use of ign_gazebo.launch.py with custom gazebo plugins

Signed-off-by: Andrej Orsula <orsula.andrej@gmail.com>
@chapulina chapulina added this to Inbox in Core development via automation Dec 4, 2020
@chapulina chapulina added the ROS 2 ROS 2 label Dec 4, 2020
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I agree that we shouldn't overwrite the variable.

I am failing to find a mention of LD_LIBRARY_PATH with respect to plugins in Finding resources

Good point, I think it could be mentioned. LD_LIBRARY_PATH is a Linux variable and tools like colcon set it. In general, Ignition libraries try to respect LD_LIBRARY_PATH on Linux and DYLD_LIBRARY_PATH on macOS.

Core development automation moved this from Inbox to In review Dec 4, 2020
@chapulina chapulina merged commit 57f5bf5 into gazebosim:ros2 Dec 4, 2020
Core development automation moved this from In review to Done Dec 4, 2020
@AndrejOrsula AndrejOrsula deleted the ros2_launch_plugin_path_extend branch December 5, 2020 18:05
@AndrejOrsula
Copy link
Contributor Author

In general, Ignition libraries try to respect LD_LIBRARY_PATH on Linux

That's great.

LD_LIBRARY_PATH is still not searched automatically when loading plugins using ign gazebo example.sdf though. I assume that's by design and using a separate environment variable is a common practice for locating shared objects in plugin-based architectures?

@chapulina
Copy link
Contributor

LD_LIBRARY_PATH is still not searched automatically when loading plugins using ign gazebo example.sdf though.

Interesting, I assumed it would work, but it doesn't. The only drawback I can see is if the user has some Gazebo classic plugin installed in LD_LIBRARY_PATH with the same name as an Ignition plugin, and the simulator tries to load the wrong one by mistake.

using a separate environment variable is a common practice for locating shared objects in plugin-based architectures?

That's certainly something we've been doing on Ignition, but there are projects that locate plugins in other ways. For example, enforcing that all plugins be installed to a given directory.

@AndrejOrsula AndrejOrsula restored the ros2_launch_plugin_path_extend branch December 8, 2020 17:31
@AndrejOrsula AndrejOrsula changed the title (ROS 2) Extend plugin path instead of overwriting it in ign_gazebo launch script [ros2] Extend plugin path instead of overwriting it in ign_gazebo launch script Dec 8, 2020
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ROS 2 ROS 2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants