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

[ROS 2] Support modules on OSX with .so or .bundle suffix #200

Open
sloretz opened this issue Aug 19, 2020 · 0 comments
Open

[ROS 2] Support modules on OSX with .so or .bundle suffix #200

sloretz opened this issue Aug 19, 2020 · 0 comments

Comments

@sloretz
Copy link
Contributor

sloretz commented Aug 19, 2020

I ran into an issue on ros2/urdf#13 where pluginlib is unable to find a module on only OSX. The cause is I buit the library using the MODULE type instead of the SHARED type, and that caused the built library name to be liburdf_xml_parser.so instead of liburdf_xml_parser.dylib.

add_library(urdf_xml_parser MODULE
  src/urdf_plugin.cpp
)

https://github.com/ros2/urdf/pull/13/files#diff-a6cb7a77ca27d5dcfb7fe4ec2004f324R52-R54

It seems like MODULE would be more correct for pluginlib plugins.

SHARED libraries are linked dynamically and loaded at runtime. MODULE libraries are plugins that are not linked into other targets but may be loaded dynamically at runtime using dlopen-like functionality

On OSX Shared Libraries and Loadable Modules are different. Summarizing from the link above: both can be loaded through the dyld api, but loadable modules can't be directly linked against, and shared libraries can't be dynamically unloaded. This means it's advantageous to use MODULE in cases where one wants plugins to be unloaded. The practical difference to pluginlib is loadable modules have different file extensions on OSX. CMake creates modules with the file extension .so, but Apple apparently recommends an extension of .bundle (though I only found second hand references to this recommendation). Pluginlib should search for libraries with all of these extensions.

I can think of a few paths forward

Option 1 Embed the path in the ament index entry

// TODO(wjwwood): probably should avoid "searching" and just embed the
// relative path to the libraries in the ament index, since CMake knows it
// at build time...

This option seems preferable to me because OSX allows modules to have any file extension. If it's known at build time, then no platform specific knowledge is needed in code, and it will take less time to load the module because it's location will be known right away.

Option 2 Add std::vector<std::string> rcpptuils_get_platform_module_names()

This would be a new utility that returned lib???.so, lib???.dylib, and lib???.bundle. It would be called adjacent to the blocks below to add to the search paths.

std::vector<std::string> all_relative_library_paths = {
rcpputils::get_platform_library_name(library_name),
rcpputils::get_platform_library_name(library_name_alternative),
rcpputils::get_platform_library_name(stripped_library_name),
rcpputils::get_platform_library_name(stripped_library_name_alternative)
};
std::vector<std::string> all_relative_debug_library_paths = {
rcpputils::get_platform_library_name(library_name, true),
rcpputils::get_platform_library_name(library_name_alternative, true),
rcpputils::get_platform_library_name(stripped_library_name, true),
rcpputils::get_platform_library_name(stripped_library_name_alternative, true)

Option 3 Recommend SHARED instead of MODULE

This is what we do in practice now, but I think this is the least desirable option because SHARED libraries can't be unloaded on OSX.

See also:
https://stackoverflow.com/questions/2339679/what-are-the-differences-between-so-and-dylib-on-osx

sloretz added a commit to ros2/urdf that referenced this issue Aug 19, 2020
Fixes OSX failing build
ros/pluginlib#200

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>
sloretz added a commit to ros2/urdf that referenced this issue Sep 18, 2020
* Make urdf plugable

This makes the urdf package load parser plugins.
It includes a new api might_handle() that returns a score for how likely
the plugin is to be the one the given file format is meant for.

This change also makes the urdf xml parser a plugin instead of a special
case that is called directly.

This breaks ABI with urdf::Model because the model now stores the class
loader instance.

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>

* Restore dependency on urdfdom

Signed-off-by: Shane Loretz <[email protected]>

* Style

Signed-off-by: Shane Loretz <[email protected]>

* Stops crash; not sure why TODO

Signed-off-by: Shane Loretz <[email protected]>

* Add benchmark showing plugin overhead

Signed-off-by: Shane Loretz <[email protected]>

* Whitespace

Signed-off-by: Shane Loretz <[email protected]>

* CMake 3.5

Signed-off-by: Shane Loretz <[email protected]>

* Include <string>

Signed-off-by: Shane Loretz <[email protected]>

* Remove buildtool_export on ament_cmake

Signed-off-by: Shane Loretz <[email protected]>

* exec_depend -> build_export_depend urdfdom headers

Signed-off-by: Shane Loretz <[email protected]>

* Remove commented code

Signed-off-by: Shane Loretz <[email protected]>

* Document urdf_parser_plugin usage

Signed-off-by: Shane Loretz <[email protected]>

* Document PIMPL forward declaration

Signed-off-by: Shane Loretz <[email protected]>

* Alphabetize dependencies

Signed-off-by: Shane Loretz <[email protected]>

* Use tinyxml2 to reduce false positive might_handle()

Signed-off-by: Shane Loretz <[email protected]>

* Update urdf/src/urdf_plugin.cpp

Signed-off-by: Shane Loretz <[email protected]>

Co-authored-by: Chris Lalancette <[email protected]>

* Handle pluginlib exceptions

Signed-off-by: Shane Loretz <[email protected]>

* Return early on failure

Signed-off-by: Shane Loretz <[email protected]>

* Document size_t max is no confidence score

Signed-off-by: Shane Loretz <[email protected]>

* Remove debut print

Signed-off-by: Shane Loretz <[email protected]>

* Move urdfdom_headers comment one line below

Signed-off-by: Shane Loretz <[email protected]>

* Avoid using nullptr in release mode

Signed-off-by: Shane Loretz <[email protected]>

* nonvirtual dtor final class

Signed-off-by: Shane Loretz <[email protected]>

* Use ROS 2 urdfdom_headers

Signed-off-by: Shane Loretz <[email protected]>

* Remove unused exec variable

Signed-off-by: Shane Loretz <[email protected]>

* Skip if xml fails to parse

Signed-off-by: Shane Loretz <[email protected]>

* Make sure test can find pluginlib plugin

Signed-off-by: Shane Loretz <[email protected]>

* Use SHARED instead of module

Fixes OSX failing build
ros/pluginlib#200

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>

* picked -> chosen

Signed-off-by: Shane Loretz <[email protected]>

* Use pluginlib_enable_plugin_testing()

Signed-off-by: Shane Loretz <[email protected]>

* Define might_handle() return to length of data

Signed-off-by: Shane Loretz<[email protected]>
Signed-off-by: Shane Loretz <[email protected]>

* Return data.size() when not confident

Signed-off-by: Shane Loretz <[email protected]>

* Try urdf when no plugin is confident

Signed-off-by: Shane Loretz <[email protected]>

* ModelImplementation final

Signed-off-by: Shane Loretz <[email protected]>

* Initialize best_plugin to nullptr

Signed-off-by: Shane Loretz <[email protected]>

Co-authored-by: Chris Lalancette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant