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

Add rclcpp_components::component target #1855

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 4, 2022

Currently any rclcpp component is forced to depend on and link against the component manager. Plugins actually just need access to rclcpp_component's headers which themselves use rclcpp and class_loader. This adds and exports a new CMake interface library rclcpp_components::component that can be depended upon by components.

Part of ament/ament_cmake#365

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz self-assigned this Jan 4, 2022
@sloretz
Copy link
Contributor Author

sloretz commented Jan 4, 2022

CI (fastrtps only for shorter CI, build: --packages-above-and-dependencies rclcpp_components test: --packages-above rclcpp_components)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Jan 5, 2022

This seems kind of weird to me, since the headers on the path included aren't all header-only (from what I can see). It would be easy to misuse this target to get access to the headers, use a non-header only one, and then fail to link to the shared library.

It kind of seems like, if you need just the headers some times and really don't want the shared library (and associated headers), then those should be in separate targets or even cmake projects.

What's the downside to linking to the shared library?

That being said, these changes look correct.

@sloretz
Copy link
Contributor Author

sloretz commented Jan 5, 2022

This seems kind of weird to me, since the headers on the path included aren't all header-only (from what I can see). It would be easy to misuse this target to get access to the headers, use a non-header only one, and then fail to link to the shared library.

I think this is less weird than it seems. The target only says which directory to look for headers, it doesn't say what headers are part of it. Not even separate CMake projects can avoid this because in a merged workspace that directory is /opt/ros/<distro>/include. When a project depends on rclcpp::rclcpp from /opt/ros/... then it can also include headers from any other installed package (say #include <cartographer/common/lua.h>) and fail to link.

What's the downside to linking to the shared library?

Weirdness 🙃. Right now the only way to use modern CMake with rclcpp_components when writing a component is to link against rclcpp_components::component_manager, which I think looks weird. I suspect it would also improve compilation time since the linker would have fewer unused libraries to look for symbols in. I also read a page that says it avoids recompiling libraries when dependencies they don't use change, so changes to component_manager or dependencies composition_interfaces won't cause all plugins in a workspace to be rebuilt (as long as they use rclcpp_components::component instead of ament_target_dependencies()).

@wjwwood
Copy link
Member

wjwwood commented Jan 6, 2022

Ok, I mean, I'm not really convinced by the compiler speed up idea, but I do get the "weirdness".

I still kind of think that if these two things are so different maybe they should in a separate packages or separate include folders, but I get why that's annoying or weird to do as well.

@@ -20,6 +20,15 @@ find_package(composition_interfaces REQUIRED)
find_package(rclcpp REQUIRED)
find_package(rcpputils REQUIRED)

# Add an interface library that can be dependend upon by libraries who register components
add_library(component INTERFACE)
Copy link
Member

Choose a reason for hiding this comment

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

Also, should this target name be something like component_interface or something? We have "libcomponent_manager" and "libcomponent", but this is an "interface library". Maybe it's redundant, but I just wanted to bring it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, should this target name be something like component_interface

I would recommend against adding interface because downstream users don't need to know that to use it. For example I'm pretty sure Eigen3::Eigen is an interface library instead of a shared one, but regardless I'm going to pass it into a call to target_link_libraries. Leaving off the interface suffix leaves the door open to making this a shared library in the future too.

@sloretz sloretz merged commit 9583ec7 into master Jan 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the rclcpp_components_export_component_target branch January 6, 2022 22:31
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.

None yet

2 participants