-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix overriding of install #13
Conversation
54e38fa
to
574f521
Compare
Signed-off-by: Tyler Weaver <tyler@picknik.ai>
574f521
to
8dc3417
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.
This is a quite opinionated review. I would like to discuss this here before we continue with other things.
6f06a77
to
a781424
Compare
@destogl I pushed a change restoring the THIS_PACKAGE_INCLUDE_DEPENDS list and using ament_target_dependencies instead of target_link_libraries. Do you prefer it like this? |
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.
Yes I like ti very much! Thanks for following up!
Let's wait to see what @bmagyar thinks about this before going to adjust all other PRs.
Signed-off-by: Tyler Weaver <tyler@picknik.ai>
a781424
to
3ad2264
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.
This is very neat guys, thanks a lot!
I'll fix the format job here too if you want me to. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #13 +/- ##
=========================================
Coverage ? 31.11%
=========================================
Files ? 2
Lines ? 180
Branches ? 116
=========================================
Hits ? 56
Misses ? 20
Partials ? 104
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I fixed it in another PR and updated all the hooks. So would be great if you could rebase this when I merge another when then I'll merge this |
OK, I merge master here. Now we have very clean PR. If CI is happy I am merging this! |
This PR cleans up a few things about the CMake but mostly it applies a fix for this: https://colcon.readthedocs.io/en/released/user/overriding-packages.html#how-to-make-it-easier-for-your-users-to-override
This is related to: