Create an aggregate target for rosidl generated interfaces targets#947
Create an aggregate target for rosidl generated interfaces targets#947
Conversation
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
emersonknapp
force-pushed
the
emerson/rosidl_interfaces_group_target
branch
from
966a6ef to
2e3afec
Compare
|
Pulls: #947, ros2/rcl#1302
|
|
@Mergifyio backport kilted jazzy humble |
✅ Backports have been createdDetails
|
) Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit e61e46d)
) Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit e61e46d)
) Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit e61e46d)
|
This is great, thanks! I guess now it is also possible to adjust the deprecation warning for ament_target_dependencies in distros that still have it, right? |
| # Create a convenience aggregate target @PROJECT_NAME@::@PROJECT_NAME@ | ||
| # that links all generated interface targets, so downstream packages can use | ||
| # a single modern CMake target name instead of ${@PROJECT_NAME@_TARGETS}. | ||
| if(@PROJECT_NAME@_TARGETS AND NOT TARGET @PROJECT_NAME@::@PROJECT_NAME@) |
There was a problem hiding this comment.
I know I'm late to the conversation (sorry), but given this potential (and I think real in the wild) collision, did you consider something like this:
target_link_libraries(my_lib sensor_msgs::rosidl_targets)Or something more meaningful that repeating the project name? Perhaps this could be a "yes, and ..." idea. We could have both targets defined (e.g. sensor_msgs::rosidl_targets, or whatever we call it, and sensor_msgs::sensor_msgs unless that target already exists)?
There was a problem hiding this comment.
Long term, this target could also be an aggregate for more granularity, e.g. sensor_msgs::rosidl_targets_cpp, sensor_msgs::rosidl_targets_python, sensor_msgs::rosidl_generate_protobuf_targets, etc.
There was a problem hiding this comment.
I like it, makes sense to me
There was a problem hiding this comment.
What about a more human-freindly ::interfaces ?
There was a problem hiding this comment.
Collisions is indeed a problem. And I would suggest changing the name before the next release.
Regarding the name for this interface target: I would suggest to NOT includ "target(s)" in the name as this is an interface target and not a list of targets.
Maybe something like "rosidl_interfaces" as "interfaces" could be another candidate for collisions.
|
|
||
| find_package(ament_cmake_export_dependencies QUIET REQUIRED) | ||
|
|
||
| _rosidl_cmake_aggregate_target_register_package_hook() |
There was a problem hiding this comment.
Also, there should probably be a documented way to opt-out of this behavior. Currently you could set _ROSIDL_CMAKE_AGGREGATE_TARGET_PACKAGE_HOOK_REGISTERED manually, but that doesn't seem very dog-foody.
There was a problem hiding this comment.
Why would you want to opt out of that? Targets are the way to go.
Instead "${PROJECT_NAME}_TARGETS" should be removed at some point.
6 participants
Description
Fixes #746
Adds a new aggregate target via CMake
INTERFACEtarget type to bundle together the${package_TARGETS}that are currently needed to use rosidl interfaces.can now use the following to get the same result.
Is this user-facing behavior change?
This is a new feature that users can opt in to using.
Did you use Generative AI?
This was assisted by Claude Code using Sonnet 4.6 model, all lines manually reviewed.
Additional Information
N/A