Skip to content

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 16, 2019

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/

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Copy link
Member

@nuclearsandwich nuclearsandwich left a comment

Choose a reason for hiding this comment

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

Thanks for tracking this down and the detailed PR description. ❤️

@nuclearsandwich
Copy link
Member

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
Copy link
Contributor Author

sloretz commented Jul 26, 2019

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

@nuclearsandwich
Copy link
Member

@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.

@sloretz sloretz merged commit e4e1b02 into ros2 Jul 26, 2019
@sloretz sloretz deleted the sloretz/fix-exported-library branch July 26, 2019 18:30
sloretz added a commit that referenced this pull request Jul 26, 2019
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
sloretz added a commit that referenced this pull request Jul 29, 2019
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants