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

[ROS 2] Correclty export class_loader library #129

Closed sloretz closed 5 years ago

sloretz commented 5 years ago

This fixes a bug that prevents building composable nodes using the ROS 2 dashing debian packages.

Before this PR ament_export_libraries(class_loader) is called before the library class_loader is created. This causes a check in ament_export_libraries to determine that the name is not a target, and so it calls ament_export_library_names(class_loader). That causes the name class_loader to be stored in _exported_library_names instead of _exported_libraries. Plain library names are searched using find_library() without any PATHS. This fails.

Packages that find_package(rclcpp_components) emit a cmake warning like:

CMake Warning at /opt/ros/dashing/share/class_loader/cmake/ament_cmake_export_libraries-extras.cmake:117 (message):
  Package 'class_loader' exports library 'class_loader' which couldn't be
  found
Call Stack (most recent call first):
  /opt/ros/dashing/share/class_loader/cmake/class_loaderConfig.cmake:38 (include)
  /opt/ros/dashing/share/rclcpp_components/cmake/ament_cmake_export_dependencies-extras.cmake:21 (find_package)
  /opt/ros/dashing/share/rclcpp_components/cmake/rclcpp_componentsConfig.cmake:38 (include)
  CMakeLists.txt:8 (find_package)

and fail to build composable nodes with linker errors

libsnap_nav.so: undefined reference to `class_loader::impl::getCurrentlyLoadingLibraryName[abi:cxx11]()'
libsnap_nav.so: undefined reference to `class_loader::impl::getPluginBaseToFactoryMapMapMutex()'
libsnap_nav.so: undefined reference to `class_loader::impl::hasANonPurePluginLibraryBeenOpened(bool)'
libsnap_nav.so: undefined reference to `class_loader::impl::getCurrentlyActiveClassLoader()'
libsnap_nav.so: undefined reference to `class_loader::impl::AbstractMetaObjectBase::setAssociatedLibraryPath(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
libsnap_nav.so: undefined reference to `class_loader::impl::getFactoryMapForBaseClass(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
libsnap_nav.so: undefined reference to `class_loader::impl::AbstractMetaObjectBase::AbstractMetaObjectBase(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
libsnap_nav.so: undefined reference to `class_loader::impl::AbstractMetaObjectBase::addOwningClassLoader(class_loader::ClassLoader*)'
collect2: error: ld returned 1 exit status

This PR fixes it by moving the call to ament_export_libraries() to after where the target is created. That changes the logic downstream packages use to find_library(class_loader) to be one that specifies paths to class_loader's lib directory.

I have no idea why the build farm didn't catch this. I think I would have expected this job to be failing but it's not. Something is causing find_library(class_loader) to succeed in the build farm, but not when I as a user do find_package(rclcpp_components). The library path is /opt/ros/dashing/lib/libclass_loader.so, so maye a different package is adding /opt/ros/dashing/lib to CMAKE_LIBRARY_PATH in it's <project>Config.cmake?

Also, the logic in ament_export_libraries() should probably be moved to the package hook. I'll make a separate PR for that.

I think it might also be the cause of https://answers.ros.org/question/327691/building-components-from-clion-doesnt-work/

nuclearsandwich commented 5 years ago

so maybe a different package is adding /opt/ros/dashing/lib

When building binary packages the ros_workspace package, which provides the /opt/ros/$ROSDISTRO/ root setup files also sets some "sane defaults" like adding bin to PATH, lib to LD_LIBRARY_PATH, and the python module directory to PYTHONPATH. So the buildfarm does indeed mask this problem.

sloretz commented 5 years ago

@nuclearsandwich OK if I merge this? I'd like to get it into the dashing patch release.

nuclearsandwich commented 5 years ago

@nuclearsandwich OK if I merge this? I'd like to get it into the dashing patch release.

Yes please do. I assumed that you would since you have the power. I will be clearer next time.