Skip to content

When using CMake >= 3.24 use CMAKE_COMPILE_WARNING_AS_ERROR variable instead of setting -Werror directly #2

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

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented Apr 8, 2025

The -Werror option is typically enabled automatically by library authors, so they ensure that all contributors to the library early fail if new code they write contain a warning, ensuring as soon as possible that no new warnings are added to the library. On the other hand, people that package libraries for distributions prefer to disable the -Werror option, as they may want to compile a given library with a new compilers (that may not have even existed when a given release of a library was tagged), that introduce new warnings, without having a failure. For a detailed discussion of this from the point of view of people packaging libraries, see https://youtu.be/_5weX5mx8hc?si=ZtiWaK7KPTfQ01_g&t=322 .

Thanks to the CMAKE_COMPILE_WARNING_AS_ERROR CMake option available since CMake 3.24, it is possible to have the best of both worlds. Library authors can define this variable as an option and have it ON by default, while people packaging libraries can just set it to OFF from CMake command line invocation (or pass --compile-no-warning-as-error option to CMake) to have build scripts that will work also with more recent compilers.

@traversaro
Copy link
Contributor Author

xref: PickNikRobotics/RSL#117

@traversaro traversaro changed the title When using CMake >= 3.24 use CMAKE_COMPILE_WARNING_AS_ERROR variable When using CMake >= 3.24 use CMAKE_COMPILE_WARNING_AS_ERROR variable instead of setting -Werror directly Apr 8, 2025
@berndpfrommer
Copy link
Contributor

closed and re-opened PR to re-run the tests w/o the broken test on Foxy.

@berndpfrommer berndpfrommer merged commit 04ae649 into ros-misc-utilities:master Apr 8, 2025
15 of 19 checks passed
@berndpfrommer
Copy link
Contributor

Thanks for the PR!

@traversaro
Copy link
Contributor Author

Thanks for the merge!

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.

2 participants