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

Update generate_parameter_library dependency in steering_controllers_library #1465

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Jan 2, 2025

Technically the generate_parameter_library is a build dependency, but it generate an header and a target that are part of the public interface of the library, and that depend on a series of CMake targets (fmt::fmt, rsl::rsl and more, see https://github.com/PickNikRobotics/generate_parameter_library/blob/0346fde52ba515593bd51b96bc520fa872af5b2a/generate_parameter_library/cmake/generate_parameter_library.cmake#L85) that are not listed as dependency of the steering_controllers_library package. The easiest solution to ensure that all the transitive dependencies of the code generated by generate_parameter_library are available is to simply add generate_parameter_library as a regular dependency, that is also the solution suggested by generate_parameter_library documentation.

Checklist:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.83%. Comparing base (2375aca) to head (ba964d9).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1465   +/-   ##
=======================================
  Coverage   83.83%   83.83%           
=======================================
  Files         122      122           
  Lines       11120    11121    +1     
  Branches      944      945    +1     
=======================================
+ Hits         9322     9323    +1     
  Misses       1489     1489           
  Partials      309      309           
Flag Coverage Δ
unittests 83.83% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

@christophfroehlich christophfroehlich changed the title Move generate_parameter_library from build_depend to depend in package.xml in steering_controllers_library Update generate_parameter_library dependency in steering_controllers_library Jan 2, 2025
@christophfroehlich christophfroehlich added the backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble. label Jan 2, 2025
Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

We have it as build_dependency in several other packages in this repo, but only this library here is really meant to be used by other controllers.

@christophfroehlich christophfroehlich merged commit ad7739f into ros-controls:master Jan 2, 2025
24 of 25 checks passed
mergify bot pushed a commit that referenced this pull request Jan 2, 2025
christophfroehlich pushed a commit that referenced this pull request Jan 2, 2025
@traversaro
Copy link
Contributor Author

We have it as build_dependency in several other packages in this repo, but only this library here is really meant to be used by other controllers.

Sorry for the late feedback. Good catch, I did not noticed those. Indeed, if you never call find_package(<package>) name for those packages, we will never experience the error that this PR fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble This label should be used by maintainers only! Label triggers PR backport to ROS2 humble.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants