Add rclcpp_components::component target#1855
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
|
CI (fastrtps only for shorter CI, build:
|
|
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. |
| 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.
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.
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 userclcppandclass_loader. This adds and exports a new CMake interface libraryrclcpp_components::componentthat can be depended upon by components.Part of ament/ament_cmake#365