GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-112301: Enable warning emitting options and ignore warnings in CI by nohlson · Pull Request #123020 · python/cpython

Conversation

@nohlson

@nohlson nohlson commented Aug 14, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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:

  • Clang output parser can pull out the option that triggered the warning
  • A path prefix argument to the script that will be lstrip() from the file path parsed from the compiler output
  • For each file with unexpected warnings a summary will be printed with the number of warnings found vs. expected
  • For each file with unexpected improvements a summary will be printed with the number of warnings found vs. expected

Compiler options enabled:

  • -Wconversion
  • -Wimplicit-fallthrough
  • -Werror=format-security
  • -Wbidi-chars=any
  • -Wall

This 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:

  • macos-14 non-free-threading
  • ubuntu non-free-threading

All warnings have been added to the warning ignore files for the respective platforms.

This is one step in the roadmap for compiler hardening.

  • Issue: Consider applying flags for warnings about potential security issues #112301

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
devdanzin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
nohlson and others added 30 commits July 23, 2024 03:14
@nohlson
Create simple warning check tool and add to ubuntu build and test job
5148727
@nohlson
Add flags to check warnings script to fail on regression or improvement
3bd5a10
@nohlson
Remove redundant comment
6813003
@nohlson
Rename warnigore file to warningignore
ab4d754
@nohlson
Use regex to extract json arrays
615d228
@nohlson
Trim whitespace
fc0a60b
@nohlson
Test on github unexpected improvement
7793d80
@nohlson
Add config for improve fail check
99715d2
@nohlson
Revert to prod check warning state
bc44ec2
@blurb-it @nohlson
📜🤖 Added by blurb_it.
e2ca75f
@nohlson
Refactor creating set of files with warnings to a dedicated function
bd1634e
@nohlson
Move cflags configure option to top level build configuration
b07b1d6
@nohlson
Add json diagnostics to ubuntu configuration as first argument
e1954a5
@nohlson
Add newline to news
5935143
@nohlson
Create simple warning check tool and add to ubuntu build and test job
7f1a238
@nohlson
Add macos warning checks to GitHub actions
144136e
@nohlson
Revert reusable-macos.yml for environment variables
3dd40fd
@nohlson
Update paths
1158f53
@nohlson
Test unexpected improvement
cb51b4f
@nohlson
Remove warning ignore
77e0f6e
@nohlson
Add json output option to macos configure job
3e1d75f
@nohlson
Add common dictionary format when parsing warnings
b5cd58a
@nohlson
Remove configure option for macos job
02f313e
@nohlson
Print out json version of compiler output
83d1ed7
@nohlson
Remove old version of warning ignore file
08a6f6d
@nohlson
Remove compiler output print diagnostic
7be8ee6
@blurb-it
📜🤖 Added by blurb_it.
8b0a2ee
@nohlson
Remove superfluous comment
49cbd87
@nohlson
oMerge branch 'main' into add-macos-warnings-tracking-tooling
522f27c
@nohlson
Add period to news
b654a84
nohlson added 2 commits August 27, 2024 17:32
@nohlson
Remove conversion warning
2e666c2
@nohlson
Update macos warning ignore
e5d0bee
sethmlarson
sethmlarson reviewed Aug 28, 2024
View reviewed changes

@sethmlarson sethmlarson left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nohlson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@sethmlarson
sethmlarson requested review from erlend-aasland and hugovk August 28, 2024 15:24
nohlson added 4 commits August 28, 2024 10:48
@nohlson
Reintroduce -Wconversion
b2cfe15
@nohlson
Merge branch 'main' into add-warning-options-and-include-in-check-war…
3e331ea 
…nings
@nohlson
Change gcc compiler output parsing to not use json
98851b6
@nohlson
Update ubuntu workflow to not output json
51c39df
hugovk
hugovk reviewed Aug 29, 2024
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/build/.warningignore_macos Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/build/.warningignore_macos Resolved

Uh oh!

There was an error while loading. Please reload this page.
nohlson added 6 commits August 29, 2024 12:56
@nohlson
Update ubuntu warning ignore list for regular GCC compiler output
3e3b81c
@nohlson
Remove leading space from macos warning ignore file
db5d6bb
@nohlson
Merge branch 'main' into add-warning-options-and-include-in-check-war…
b82355f 
…nings
@nohlson
Update warning ignore files for new warnings from main
bf90af1
@nohlson
Remove zero count files from warning ignore
9701292
@nohlson
Remove redundant line in warningignore
0cd2731
@nohlson

nohlson commented Sep 11, 2024

Copy link
Copy Markdown
Contributor Author

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?

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?

The warning ignore files need to be updated immediately before merging. This is because any warnings that have merged to main since the last time main was pulled in will introduce warning diffs.
sethmlarson and hugovk reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hugovk added 2 commits September 13, 2024 16:02
@hugovk
Merge branch 'main' into add-warning-options-and-include-in-check-war…
71b0617 
…nings
@hugovk
Update warning ignore files for new warnings from main
51782d5
hugovk
hugovk approved these changes Sep 13, 2024
View reviewed changes

@hugovk hugovk left a comment

Copy link
Copy Markdown
Member

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 merging main 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sethmlarson reacted with heart emoji
All reactions
  • ❤️ 1 reaction
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Sep 13, 2024
@hugovk
hugovk enabled auto-merge (squash) September 13, 2024 13:34
@hugovk
hugovk merged commit cfe6074 into python:main Sep 13, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 13, 2024
@sobolevn sobolevn mentioned this pull request Sep 13, 2024
Lots of new compiler warnings #124064
Closed
hugovk added a commit to hugovk/cpython that referenced this pull request Sep 13, 2024
@hugovk
Revert "pythongh-112301: Enable warning emitting options and ignore w…
e0e278e 
…arnings in CI (python#123020)"

This reverts commit cfe6074.
@hugovk hugovk mentioned this pull request Sep 13, 2024
Revert "gh-112301: Enable warning emitting options and ignore warnings in CI (#123020)" #124065
Merged
hugovk added a commit that referenced this pull request Sep 13, 2024
@hugovk
Revert "gh-112301: Enable warning emitting options and ignore warning…
ea77973 
…s in CI (#123020)" (#124065)
@hugovk hugovk mentioned this pull request Apr 12, 2026
CI: Move Homebrew dependencies into Brewfile #148335
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sethmlarson sethmlarson sethmlarson left review comments
@hugovk hugovk hugovk approved these changes
@corona10 corona10 Awaiting requested review from corona10 corona10 is a code owner
@ezio-melotti ezio-melotti Awaiting requested review from ezio-melotti ezio-melotti is a code owner
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland erlend-aasland is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@nohlson @hugovk @erlend-aasland @sethmlarson
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.