Use target_link_libraries instead of ament_target_dependencies

[wep21]

Use target_link_libraries instead of ament_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

[emersonknapp]

Can you explain why you are making this change? Is there a new recommendation from ament_cmake that you use target_link_libraries? This is not more "modern" - target_link_libraries already existed when ament_cmake was first written, ament_target_dependencies does more than just library linking.

Without a strong justification and context for this change, I am not inclined to move forward.

[wep21]

@emersonknapp Please see this issue.

[emersonknapp]

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?
Please add this context to the PR description, we need to know "why?" in addition to "what?"

[clalancette]

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 ament_target_dependencies in many places in the core code, and in many more places in the wider ecosystem. It isn't going anywhere anytime soon, but its core functionality mostly does overlap with target_link_libraries. So we are (slowly) changing over the core to just use target_link_libraries where we can, which is likely where this came from.

[emersonknapp]

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"

[emersonknapp]

Just from a nitpicky smell and flavor perspective, I'll note that I don't love that now you have to repeat yourself with package::package naming. But, you know, whatever. Not every nutritious thing tastes good.

[emersonknapp]

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!

[clalancette]

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.

[MichaelOrlov]

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.

[wep21]

@emersonknapp @MichaelOrlov @clalancette I rebased the branch from latest rolling. Please review again.

[emersonknapp]

OK - it seems fine with green CI

[MichaelOrlov]

@wep21 Please consider to address one nitpick.
And I have a couple questions about missing target dependencies. Please see them in dedicated comments.

[MichaelOrlov]

@wep21 Thanks for the PR.
LGTM now.

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/759beca6bdc37394a5b4cbaa6b22736b/raw/d3221f0176000106c912c1f23ab407c4a7830804/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_compression_zstd rosbag2_transport rosbag2_cpp rosbag2_examples_cpp rosbag2_performance_benchmarking rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_test
TEST args: --packages-above rosbag2_compression rosbag2_compression_zstd rosbag2_transport rosbag2_cpp rosbag2_examples_cpp rosbag2_performance_benchmarking rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_test
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11478

• Linux
• Linux-aarch64
• Windows

[MichaelOrlov]

Gist: https://gist.githubusercontent.com/MichaelOrlov/8ef3c7b32ac9aac982f412fdc29bae53/raw/d3221f0176000106c912c1f23ab407c4a7830804/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_compression_zstd rosbag2_transport rosbag2_cpp rosbag2_examples_cpp rosbag2_performance_benchmarking rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_compression_zstd rosbag2_transport rosbag2_cpp rosbag2_examples_cpp rosbag2_performance_benchmarking rosbag2_py rosbag2_storage rosbag2_storage_mcap rosbag2_storage_sqlite3 rosbag2_test_common rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11480

• Linux
• Linux-aarch64
• Windows