-
Notifications
You must be signed in to change notification settings - Fork 242
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
Use target_link_libraries instead of ament_target_dependencies #1202
Conversation
06f9538
to
06e959b
Compare
Can you explain why you are making this change? Is there a new recommendation from Without a strong justification and context for this change, I am not inclined to move forward. |
@emersonknapp Please see this issue. |
ament/ament_cmake#292 is not closed, and I'm not seeing a documentation update changing recommendations for downstream packages. Unless I am missing it and there has been such a documentation update? |
There is a documentation update in the works: ros2/ros2_documentation#2915 . We still use |
Thanks for the link to the docs. I have updated the PR description with all the context. Just skeptical of changes without an obvious reason - I'm fine moving forward with this, maybe delaying to "depends on ros2/ros2_documentation#2915" |
Just from a nitpicky smell and flavor perspective, I'll note that I don't love that now you have to repeat yourself with |
yaml_cpp_vendor | ||
target_link_libraries(writer_benchmark | ||
rclcpp::rclcpp | ||
${std_msgs_TARGETS} |
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.
Actually - this makes me very suspicious of this change! As an end user, how am I supposed to know when I need ${pkg_TARGETS}
vs pkg::pkg
vs {pkg_LIBRARIES}
? This seems strictly worse!
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.
So this is a bug in the current CMake code that is generated for message packages (and is one of the main reasons this has been a "soft" changeover so far).
That is, we really, really want this to be std_msgs::std_msgs
, but we have so far been unable to make that work properly (and we have tried). For now, the workaround has been to stick with x_msgs_TARGETS
for all msgs packages, and use modern CMake targets for everything else.
Whether you want to reject this PR because of that, I'll leave to you. I'll point out that we are accepting this elsewhere in the core for now.
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.
It seems migration out of ament_target_dependencies
unavoidable since it will become deprecated soon.
I also don't like a workaround with ${test_msgs_TARGETS}
and another messages files, but I am tend to incline to make incremental fixes rather than waiting when it will be solved thoroughly.
Overall movements towards standard cmake target_link_libraries
looks reasonable, however I am bit concerned that relevant PR ros2/ros2_documentation#2915 for official documentation update is sort of in stale stage and hasn't been moved forward or updated for a 4 months.
rosbag2_performance/rosbag2_performance_benchmarking/CMakeLists.txt
Outdated
Show resolved
Hide resolved
rosbag2_performance/rosbag2_performance_benchmarking/CMakeLists.txt
Outdated
Show resolved
Hide resolved
rosbag2_performance/rosbag2_performance_benchmarking/CMakeLists.txt
Outdated
Show resolved
Hide resolved
a8bd934
to
7c8e43a
Compare
7c8e43a
to
2f98d6e
Compare
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
c736a97
to
b031acf
Compare
@emersonknapp @MichaelOrlov @clalancette I rebased the branch from latest rolling. Please review again. |
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.
OK - it seems fine with green CI
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.
@wep21 Please consider to address one nitpick.
And I have a couple questions about missing target dependencies. Please see them in dedicated comments.
Co-authored-by: Michael Orlov <morlovmr@gmail.com> Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
41ce10e
to
f18b6c7
Compare
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.
@wep21 Thanks for the PR.
LGTM now.
Gist: https://gist.githubusercontent.com/MichaelOrlov/759beca6bdc37394a5b4cbaa6b22736b/raw/d3221f0176000106c912c1f23ab407c4a7830804/ros2.repos |
Gist: https://gist.githubusercontent.com/MichaelOrlov/8ef3c7b32ac9aac982f412fdc29bae53/raw/d3221f0176000106c912c1f23ab407c4a7830804/ros2.repos |
Use
target_link_libraries
instead ofament_target_dependencies
for modern cmake.See discussion on ament/ament_cmake#292 for context on this decision.
This method will be the new standard recommendation after merge of ros2/ros2_documentation#2915