Skip to content
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

Merged
merged 4 commits into from
Jan 26, 2023
Merged

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Jan 25, 2023

@tylerjw tylerjw force-pushed the cmake_install_fix branch 4 times, most recently from 54e38fa to 574f521 Compare January 25, 2023 22:56
Signed-off-by: Tyler Weaver <tyler@picknik.ai>
Copy link
Member

@destogl destogl left a 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.

kinematics_interface/CHANGELOG.rst Outdated Show resolved Hide resolved
kinematics_interface/CMakeLists.txt Show resolved Hide resolved
kinematics_interface/CMakeLists.txt Outdated Show resolved Hide resolved
kinematics_interface/CMakeLists.txt Outdated Show resolved Hide resolved
kinematics_interface/CMakeLists.txt Show resolved Hide resolved
kinematics_interface/CMakeLists.txt Outdated Show resolved Hide resolved
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 26, 2023

@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?

Copy link
Member

@destogl destogl left a 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.

kinematics_interface/CMakeLists.txt Show resolved Hide resolved
Signed-off-by: Tyler Weaver <tyler@picknik.ai>
Copy link
Member

@bmagyar bmagyar left a 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!

@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 26, 2023

I'll fix the format job here too if you want me to.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@01a2cda). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 3ad2264 differs from pull request most recent head 069c91e. Consider uploading reports for the commit 069c91e to get more accurate results

📣 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           
Flag Coverage Δ
unittests 31.11% <0.00%> (?)

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.

@destogl
Copy link
Member

destogl commented Jan 26, 2023

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

@destogl
Copy link
Member

destogl commented Jan 26, 2023

OK, I merge master here. Now we have very clean PR. If CI is happy I am merging this!

@destogl destogl merged commit 51221b9 into ros-controls:master Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants