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

gh-112301: Enable warning emitting options and ignore warnings in CI #123020

Merged

Conversation

nohlson
Copy link
Contributor

@nohlson nohlson commented Aug 14, 2024

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.

nohlson and others added 30 commits July 23, 2024 03:14
Copy link
Contributor

@sethmlarson sethmlarson left a 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.

Copy link
Member

@hugovk hugovk left a 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?

Tools/build/.warningignore_macos Outdated Show resolved Hide resolved
Tools/build/.warningignore_macos Outdated Show resolved Hide resolved
@nohlson
Copy link
Contributor Author

nohlson commented Sep 11, 2024

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.

Copy link
Member

@hugovk hugovk left a 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!

@hugovk hugovk enabled auto-merge (squash) September 13, 2024 13:34
@hugovk hugovk merged commit cfe6074 into python:main Sep 13, 2024
38 of 39 checks passed
hugovk added a commit to hugovk/cpython that referenced this pull request Sep 13, 2024
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.

4 participants