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

build: Treat all warnings as errors #445

Merged
merged 22 commits into from
Dec 9, 2022

Conversation

plexoos
Copy link
Member

@plexoos plexoos commented Nov 18, 2022

These changes do not affect how the code is built by default using cons but allows one to interrupt compilation when the compiler (GCC 4.8.5) produces a warning.

cons EXTRA_CXXFLAGS="-Werror"

The source files matching the regex patterns in mgr/warnoff_dirs.txt are excluded from the above rule in order to let CI pass with known problems. We hope this will prevent introduction of questionable code in all other directories or new packages.

See issue #392

@plexoos plexoos requested a review from veprbl as a code owner November 18, 2022 23:57
@plexoos plexoos marked this pull request as draft November 19, 2022 00:14
mgr/ConsDefs.pm Outdated Show resolved Hide resolved
@plexoos plexoos added this to the SL22d milestone Nov 21, 2022
@plexoos plexoos marked this pull request as ready for review November 21, 2022 22:25
@plexoos plexoos requested a review from nigmatkulov as a code owner November 21, 2022 22:25
@plexoos
Copy link
Member Author

plexoos commented Nov 21, 2022

Ready for review

@plexoos
Copy link
Member Author

plexoos commented Dec 1, 2022

Any comments from the code owners?

Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

To clarify, this allows pre-existing warnings in old codes to not trigger an alert in CI, while new codes will be monitored for such warnings and either fixed or added to this exclusion list?

Is there a clear (documented?) place to make change that would allow someone compiling the whole library to see the warnings for all codes again?

@plexoos
Copy link
Member Author

plexoos commented Dec 1, 2022

Excellent questions, Gene. The answer to the first question is yes, that is how it is intended to work. With only one comment that the goal should be to shrink the exclusion list rather than adding new codes with questionable constructs. Warnings detect real bugs #431

For the second one, no special action needs to be taken, the warnings for the excluded paths will still appear in the build log, they just won't fail CI.

@plexoos
Copy link
Member Author

plexoos commented Dec 8, 2022

If no more comments can we merge?

Dockerfile Show resolved Hide resolved
@plexoos
Copy link
Member Author

plexoos commented Dec 9, 2022

@klendathu2k your last word 🥺

Copy link
Contributor

@klendathu2k klendathu2k left a comment

Choose a reason for hiding this comment

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

Should be fine, and if/when it breaks the build we will fix.

@plexoos plexoos merged commit 15a7e7c into star-bnl:main Dec 9, 2022
@plexoos plexoos deleted the pr/ignore_warnings branch December 9, 2022 15:35
@DanielWielanek
Copy link

Actually is a funny story.
For over two weeks, I had a problem with getting only the list of modified files. The script that worked on my local computer didn't work on GitHub servers. Then I found this plugin. As far as I remember, I was not able to connect both plugins correctly (I do not remember why). However, when I removed the plugin for getting the list of modified files, surprisingly, I noticed that the older script is working now. I suppose that by accident, I enabled some flags during tests :)
So in principle, this plugin actually helped me :)

@plexoos plexoos mentioned this pull request Dec 9, 2022
@plexoos
Copy link
Member Author

plexoos commented Dec 9, 2022

Cool story, wrong thread 🤣

Moved to #436 (comment)

plexoos added a commit to plexoos/star-sw that referenced this pull request Dec 15, 2022
@plexoos plexoos modified the milestones: SL22d, SL23a Jan 4, 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