-
Notifications
You must be signed in to change notification settings - Fork 251
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
Switch to target_link_libraries everywhere. #1504
Conversation
There is no need to use ament_target_dependencies. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
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.
lgtm
(not related to this PR) https://github.com/ros2/rosbag2/pull/1504/files#diff-81a326226779dbc5abab3808861f8664841bdafaed9ff22ebf59ae30b6233219L281 should be test_msgs_TARGETS
, there is a typo. can you also fix that too?
@fujitatomoya Can you paste a new link? For whatever reason, the one above isn't showing me anything other than the lines from this PR. |
@clalancette this one, rosbag2/rosbag2_transport/CMakeLists.txt Line 281 in 9855072
|
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
Actually, it turns out that that test doesn't depend on test_msgs at all, so I removed the typo along with a couple of other unused dependencies. |
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.
lgtm 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.
LGTM
@ros-pull-request-builder retest this please |
Given that the CI was green, I'm going to go ahead and merge this one. |
@clalancette It is ok to merge since the last time rpr job failed with a known issue in the services. |
There is no need to use ament_target_dependencies.