-
-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: Compiler warning management tooling #121730
Conversation
(I updated the branch from |
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.
Looking good! Here's a few comments and questions.
Tools/build/check_warnings.py
Outdated
try: | ||
json_data = json.loads(array) | ||
json_objects_in_array = [entry for entry in json_data] | ||
compiler_warnings.extend([entry for entry in json_objects_in_array if entry.get('kind') == 'warning']) |
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.
Please could you limit lines to 79 chars?
https://peps.python.org/pep-0008/#maximum-line-length
This file is in Tools/
, I think it's okay to just run Black on it:
black --line-length 79 Tools/build/check_warnings.py
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.
Broke this line into two
Tools/build/check_warnings.py
Outdated
import sys | ||
from pathlib import Path | ||
import argparse | ||
import json | ||
import re |
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.
Let's sort 'em:
import sys | |
from pathlib import Path | |
import argparse | |
import json | |
import re | |
import argparse | |
import json | |
import re | |
import sys | |
from pathlib import Path |
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.
Sorted imports
Tools/build/check_warnings.py
Outdated
for key in ['caret', 'start', 'end']: | ||
if key in location: | ||
file = location[key]['file'] | ||
file = file.lstrip('./') # Remove leading curdir if present |
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.
file = file.lstrip('./') # Remove leading curdir if present | |
file = file.lstrip('./') # Remove leading current directory if present |
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.
Changed comment
Tools/build/check_warnings.py
Outdated
for key in ['caret', 'start', 'end']: | ||
if key in location: | ||
file = location[key]['file'] | ||
file = file.lstrip('./') # Remove leading curdir if present |
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.
file = file.lstrip('./') # Remove leading curdir if present | |
file = file.lstrip('./') # Remove leading current directory if present |
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.
Changed comment
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.
Thanks @hugovk for reviewing too, I have some more comments:
I have made the requested changes; please review again |
Thanks for making the requested changes! : please review the changes made to this pull request. |
767308a
to
b07b1d6
Compare
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.
Few more comments, otherwise looks good for the first PR :)
Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst
Outdated
Show resolved
Hide resolved
import sys | ||
from pathlib import Path | ||
|
||
def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: |
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.
This looks almost good to merge, just some style nits :)
Technically we're outside the stdlib so strictly speaking PEP 8 doesn't apply, but let's follow it anyway. For example, please could you add extra newlines before these defs and wrap things to 79 chars?
The easiest way is to run Black (which we don't run on the stdlib but again we're outside the stdlib :)
black --line-length 79 Tools/build/check_warnings.py
Then some comments and long strings will need manual wrapping.
def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: | |
def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: |
Also, please don't force push in this repo, it makes it easier to review new commits, and we squash merge everything at the end anyway. I think force-pushing is sometimes to blame for pinging lots of unrelated CODEOWNERS too.
In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.
https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide
Thanks!
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.
Updated to adhere to PEP8.
After a rebase there were several commits that came from branches from different forks. Not really sure how that happened but I had to revert to my last commit and force push to get rid of them.
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.
Thank you! I just pushed a commit to also wrap comments and strings to 79 chars as well.
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.
Thanks!
@@ -0,0 +1,195 @@ | |||
#!/usr/bin/env python3 |
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.
Do we need this one?
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.
I meant to comment this too: no, I don't think we do because the file does not have the executable bit and we're running it via python Tools/build/check_warnings.py
@picnixz Your review comments came after I'd enabled auto-merge, sorry about that! @nohlson Hi! @corona10 noticed that with this change, the build output is no longer being output to the logs, because we're redirecting it to a file. It can sometimes be useful to see this output. Before: https://github.com/python/cpython/actions/runs/10122181963/job/27993976874 @zware suggested a command like this: set -o pipefail; make -j4 2>&1 | tee compiler_output.txt Please could you open a PR to try this out? Thank you! |
#112301: This is the first version of compiler warning check tooling integrated into the Ubuntu build ant test GitHub action job.
The tool parses compiler output and checks against an ignore file, which is a list of files for which warnings should be ignored. The script includes options to fail on regression, or fail on improvement (if a file is in the ignore list but the compiler emitted no warnings).
A more detailed writeup about the idea behind the tool can be found here
The current configuration in
reusable-ubuntu.yml
does not include flags to--fail-on-regression
or--fail-on-improvement
and simply writes to the log warnings that were emitted but not ignored, or file names that were unexpectedly missing warnings but will not fail the step in the job. Per discussion in the issue we can eventually enable those options.Future improvements would include removal of duplicate warnings, warning count per file tracking, and adding GitHub Actions annotations.