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

chore(nebula_ros): specify library destination for install targets #261

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

mbharatheesha
Copy link
Contributor

PR Type

Improvement

Description

This PR introduces a minimal delta needed to enable the installation of shared libraries corresponding to the composable node targets for the differet ros wrappers into /opt/ros/<distro>/lib instead of /opt/ros/<distro>/x86_64-linux-gnu/.

This installation path matters when the ROS wrappers for the lidar drivers are used as composable nodes, the relevant shared libraries are reported as unavailable. Here are some details:

I created a binary release of this package locally using bloom using the following steps:
(Do a source build of nebula_ros package following the instructions on README)

apt-get install -qqy python-bloom fakeroot dpkg-dev debhelper
cd nebula_ros
bloom-generate rosdebian
fakeroot debian/rules binary

Without the changes in this PR

The lib<lidar_type>_ros_wrapper.so files get installed under /opt/ros/<distro>/lib/x86_64-linux-gnu/. And when I try to test the composable node versions of the ROS wrappers using:

ros2 component standalone nebula_ros HesaiRosWrapper

I get the following error:

[INFO] [1738699444.539630225] [standalone_container_51b647ce1865]: Load Library: /opt/ros/jazzy/lib/libhesai_ros_wrapper.so
[ERROR] [1738699444.539908988] [standalone_container_51b647ce1865]: Failed to load library: Could not load library dlopen error: /opt/ros/jazzy/lib/libhesai_ros_wrapper.so: cannot open shared object file: No such file or directory, at ./src/shared_library.c:99

It is the same error type for other ROS wrappers too.

With the changes in this PR

The lib<lidar_type>_ros_wrapper.so files get installed under /opt/ros/<distro>/lib. And when I try to test the composable node versions of the ROS wrappers using:

ros2 component standalone nebula_ros HesaiRosWrapper

The composable node is loaded successfully

[INFO] [1738699840.335771784] [standalone_container_639fe89b495c]: Load Library: /opt/ros/jazzy/lib/libhesai_ros_wrapper.so
[INFO] [1738699840.341811197] [standalone_container_639fe89b495c]: Found class: rclcpp_components::NodeFactoryTemplate<HesaiRosWrapper>
[INFO] [1738699840.341849698] [standalone_container_639fe89b495c]: Instantiate class: rclcpp_components::NodeFactoryTemplate<HesaiRosWrapper>

Review Procedure

The errors I mention in the description are reproducible with the commands I have posted above. Then, with the changes in this PR, one can verify that the shared libraries are installed at the location desired by the composable node infrastructure i.e., /opt/ros/<distro>/lib.

Remarks

There are a few additional changes I can think of which are mostly with improving readability. For example"

  • the following lines in CMakeLists.txt of nebula_ros:
install(TARGETS hesai_ros_wrapper EXPORT export_hesai_ros_wrapper LIBRARY DESTINATION lib)
install(TARGETS velodyne_ros_wrapper EXPORT export_velodyne_ros_wrapper LIBRARY DESTINATION lib)
install(TARGETS robosense_ros_wrapper EXPORT export_robosense_ros_wrapper LIBRARY DESTINATION lib)
install(TARGETS continental_ars548_ros_wrapper EXPORT export_continental_ars548_ros_wrapper LIBRARY DESTINATION lib)
install(TARGETS continental_srr520_ros_wrapper EXPORT export_continental_srr520_ros_wrapper LIBRARY DESTINATION lib)

can be consolidated to:

install (
  TARGETS
  continental_ars548_ros_wrapper
  continental_srr520_ros_wrapper
  hesai_ros_wrapper
  robosense_ros_wrapper
  velodyne_ros_wrapper
  EXPORT export_${PROJECT_NAME}
  LIBRARY DESTINATION lib
)

ament_export_targets(export_${PROJECT_NAME})
  • the composable nodes registration in <lidar_type>_ros_wrapper.cpp could be modifed in accordance with common practice across other ROS2 packages like this:
RCLCPP_COMPONENTS_REGISTER_NODE(nebula::ros::<LidarType>RosWrapper)

And accordingly register them in the CMakeLists.txt as:

rclcpp_components_register_node(hesai_ros_wrapper
        PLUGIN "nebula::ros::HesaiRosWrapper"
        EXECUTABLE hesai_ros_wrapper_node
)
  • And perhaps also add an example launch file using composable nodes

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

Signed-off-by: Mukunda Bharatheesha <mukunda.bharatheesha@nobleo.nl>
@mbharatheesha mbharatheesha changed the title (improvement)nebula_ros: specify library destination for install targets chore(nebula_ros): specify library destination for install targets Feb 4, 2025
@mbharatheesha
Copy link
Contributor Author

mbharatheesha commented Feb 4, 2025

@mojomex It seems I cannot assign a reviewer as requested by the PR template. So, I looked at the most recent author on the nebula_ros package and I tagged you. Kindly let me know if I should do this differently.

@mojomex
Copy link
Collaborator

mojomex commented Feb 5, 2025

Thank you for the PR, I'll have a look in the coming days!

@Timple
Copy link

Timple commented Feb 11, 2025

For what it's worth, it works in practice 🙂

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.14%. Comparing base (44e6c88) to head (11f8669).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   28.89%   27.14%   -1.75%     
==========================================
  Files         105      105              
  Lines        9465     9271     -194     
  Branches     3115     2057    -1058     
==========================================
- Hits         2735     2517     -218     
- Misses       6240     6334      +94     
+ Partials      490      420      -70     
Flag Coverage Δ
differential 27.14% <ø> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mojomex mojomex left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, much appreciated!
I confirmed it's working and that it's essentially the same thing ament_cmake_auto is also doing.

@mojomex mojomex merged commit c4e9dd9 into tier4:main Feb 12, 2025
10 of 12 checks passed
@mbharatheesha
Copy link
Contributor Author

And thank you guys for this package!

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

Successfully merging this pull request may close these issues.

3 participants