Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use global cmake macros and fix gcc-10 build #1527

Merged
merged 8 commits into from
Mar 28, 2025
Merged

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Feb 5, 2025

The change is motivated from the fact that we added a compiler option, which was introduced with gcc-11. But, debian11 (the humble tier 3), has only gcc-10 per default. (We don't have CI jobs for compatibility of the rolling version on humble debian/rhel.)

#24 258.0 --- stderr: joint_state_broadcaster
#24 258.0 cc1plus: error: '-Werror=range-loop-construct': no option '-Wrange-loop-construct'

So I added a switch depending on the gcc version to add this option or not.
To make the options simpler to handle in the future, I propose to add a global cmake macro instead.
Instead, I created a new package ros2_control_cmake with ros-controls/ros2_control_cmake#1, please review there if we should rename the macros etc.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.05%. Comparing base (996f030) to head (154cf0a).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1527   +/-   ##
=======================================
  Coverage   85.05%   85.05%           
=======================================
  Files         123      123           
  Lines       11713    11710    -3     
  Branches      999      996    -3     
=======================================
- Hits         9962     9960    -2     
- Misses       1435     1437    +2     
+ Partials      316      313    -3     
Flag Coverage Δ
unittests 85.05% <ø> (+<0.01%) ⬆️

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

see 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mathias-luedtke
Copy link
Contributor

@christophfroehlich: Careful, AFAIK bloom does not "bundle" files outside of the package folders.

@christophfroehlich
Copy link
Contributor Author

oh, thanks for the hint. This means that the packages won't be built on the build farm right?

@mathias-luedtke
Copy link
Contributor

oh, thanks for the hint. This means that the packages won't be built on the build farm right?

Yes.
Bloom creates tag like this:
https://github.com/ros2-gbp/ros2_controllers-release/tree/release/rolling/velocity_controllers/4.20.0-1

The common cmake would need to get packaged.
I am not sure, if it is worth the effort for a humble tier 3.

@christophfroehlich
Copy link
Contributor Author

it's not the tier 3 I'm concerned about, I have a personal interest due to one of my projects at work ;) but could solve it differently, sure thing.
and the cmake file was just an idea in making the settings global, independent of the actual issue.

@christophfroehlich christophfroehlich marked this pull request as draft February 5, 2025 13:26
Copy link
Contributor

mergify bot commented Feb 27, 2025

This pull request is in conflict. Could you fix it @christophfroehlich?

@christophfroehlich christophfroehlich marked this pull request as ready for review March 28, 2025 06:45
@github-actions github-actions bot requested a review from anfemosa March 28, 2025 06:45
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.

Looks great

@christophfroehlich christophfroehlich merged commit d1844e6 into master Mar 28, 2025
23 of 25 checks passed
@christophfroehlich christophfroehlich deleted the cmake_include branch March 28, 2025 09:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants