-
Notifications
You must be signed in to change notification settings - Fork 436
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
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
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. |
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
Weirdness 🙃. Right now the only way to use modern CMake with rclcpp_components when writing a component is to link against |
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 userclcpp
andclass_loader
. This adds and exports a new CMake interface libraryrclcpp_components::component
that can be depended upon by components.Part of ament/ament_cmake#365