-
Notifications
You must be signed in to change notification settings - Fork 349
Modernize CMakeLists.txt to remove deprecated ament_target_dependencies (Fixes #2210) #2210
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
Conversation
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.
@LalitNarayanYadav Thank you for your comments. Just some minor comments, if they look good, you can commit them directly from UI
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
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.
Please review the changes.
Co-authored-by: Sai Kishor Kothakota <[email protected]>
@LalitNarayanYadav Please fix the failing CI jobs. Please try to install pre-commit and fix by running the |
It would also be good to give the PR a meaningful title. @LalitNarayanYadav hope you don't mind me asking but how much of the original content of this PR was AI generated? |
☝️ @LalitNarayanYadav did this build for you locally? |
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.
I'm really not happy with the deprecation of the macro...
This PR now doubles the amount of lines it touches (+127 -62) and in it's current form does not help readability or compilation time.
There is inconsistent use of ${X_targets}
, {X_targets}
and X::X
of which allow me to say the latter looks pretty bad :D
I guess the main reason for the lines of new "code" is not using THIS_PACKAGE_INCLUDE_DEPENDS
anymore. If only there was a cmake macro that allowed that...
Any suggestions?
This pull request is in conflict. Could you fix it @LalitNarayanYadav? |
Modernize CMakeLists to remove
ament_target_dependencies()
deprecation warningsThis PR updates the
CMakeLists.txt
file to replace deprecatedament_target_dependencies()
calls with moderntarget_link_libraries()
using namespaced targets, as recommended by ROS 2 on Rolling.✅ Changes
ament_target_dependencies()
usages withtarget_link_libraries()
and appropriate namespaced targets.📦 Benefits
Closes #2207