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

Using urdf/model.hpp for rolling #1978

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

nakul-py
Copy link
Contributor

@nakul-py nakul-py commented Jan 5, 2025

Description

This pull request addresses the use of the deprecated urdf/model.h header in component_parser.cpp to ensure compatibility with Rolling, Jazzy, and Humble distributions of ROS 2.

Since urdf/model.h was replaced by urdf/model.hpp in newer ROS 2 distributions starting from Rolling, this PR introduces a version-conditional include to maintain backward compatibility with older distributions.

Fixes #1977

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.27%. Comparing base (d8a21c4) to head (af79849).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1978      +/-   ##
==========================================
+ Coverage   89.21%   89.27%   +0.05%     
==========================================
  Files         130      130              
  Lines       14495    14493       -2     
  Branches     1257     1257              
==========================================
+ Hits        12932    12938       +6     
+ Misses       1093     1088       -5     
+ Partials      470      467       -3     
Flag Coverage Δ
unittests 89.27% <ø> (+0.05%) ⬆️

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

Files with missing lines Coverage Δ
hardware_interface/src/component_parser.cpp 94.73% <ø> (ø)

... and 3 files with indirect coverage changes

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@nakul-py can you fix the pre-commit?. Thanks

bmagyar
bmagyar previously approved these changes Jan 5, 2025
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.

The rclcpp version constraint is to allow compilation on humble, correct?

@christophfroehlich
Copy link
Contributor

The rclcpp version constraint is to allow compilation on humble, correct?

and on jazzy as well.

@christophfroehlich
Copy link
Contributor

@nakul-py you can run pre-commit locally, see the message in the failing checks or the contributing guidelines, or the text in the good first issue

If you are seeing this message in CI, reproduce locally with: pre-commit run --all-files.

@nakul-py
Copy link
Contributor Author

nakul-py commented Jan 6, 2025

@nakul-py can you fix the pre-commit?. Thanks

Will this works 640d2f3

@nakul-py nakul-py requested review from saikishor and bmagyar January 6, 2025 14:01
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@nakul-py you are not supposed to touch the files of the pre-commit configuration. Please revert your last commit.

As @christophfroehlich mentioned, just run pre-commit run --all-files and this should edit the files automatically and just commit the changes after running that command.

@nakul-py nakul-py requested a review from saikishor January 7, 2025 04:26
@nakul-py
Copy link
Contributor Author

nakul-py commented Jan 7, 2025

@nakul-py you are not supposed to touch the files of the pre-commit configuration. Please revert your last commit.

As @christophfroehlich mentioned, just run pre-commit run --all-files and this should edit the files automatically and just commit the changes after running that command.

Hi @saikishor I have done that. Would this works fine af79849

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Lgtm

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, this is fine now!

@christophfroehlich christophfroehlich merged commit 6e1059c into ros-controls:master Jan 7, 2025
25 checks passed
@christophfroehlich
Copy link
Contributor

@nakul-py maybe you want to take ros-controls/ros2_controllers#1469 as well, because the same changes are needed there!

@nakul-py
Copy link
Contributor Author

nakul-py commented Jan 7, 2025

@nakul-py maybe you want to take ros-controls/ros2_controllers#1469 as well, because the same changes are needed there!

Ok i will proceed.

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.

Use urdf/model.hpp instead of deprecated urdf/model.h
4 participants