Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-112301: Enable warning emitting options and ignore warnings in CI#123020
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-112301: Enable warning emitting options and ignore warnings in CI#123020
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My comments are resolved, I'll leave a +1 review.
I spoke with@nohlson privately about potentially breaking up the addition of warnings into separate pull requests so they're only "active" while we're actively remediating the individual warnings. This would minimize the chances that core developers are going to "run into" this effort slightly.
After Nate dug into it, it turns out that almost all of the existing warnings come from-Wconversion and the other options produce far fewer warnings, some of which already have pull requests out for their remediation (like-Wformat=2).
For this reason I am okay proceeding with this PR as-is as the gains from splitting up the individual options seems smaller than I originally imagined, if other devs have concerns about this approach please leave them below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We can't merge this yet because the CI is failing:
https://github.com/python/cpython/actions/runs/10609035033/job/29404117175?pr=123020
How can we fix it?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
The warning ignore files need to be updated immediately before merging. This is because any warnings that have merged to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unsurprisingly there were new warnings when mergingmain in again, but the CI error message was useful: it told me which file had a different number of warnings, and linked to the devguide instructions on how to update.
Thanks@nohlson, let's merge!
…arnings in CI (python#123020)"This reverts commitcfe6074.
Uh oh!
There was an error while loading.Please reload this page.
This PR introduces new compiler options that only emit warnings and don't affect runtime performance and some improvements to the warning check tooling.
Warning check tooling improvements:
lstrip()from the file path parsed from the compiler outputCompiler options enabled:
-Wconversion-Wimplicit-fallthrough-Werror=format-security-Wbidi-chars=any-WallThis PR also modifies the reusable-macos and reusable-ubuntu jobs so that compiler output is only written to a file and warning checking is only performed on the specific jobs that we want:
All warnings have been added to the warning ignore files for the respective platforms.
This is one step in theroadmap for compiler hardening.