Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork931
Add compiler warning tracking tooling failure remediation guidance#1364
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
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.
Thanks! Some suggestions :)
Tip: You can apply all desired suggestions with one click by going to theFiles changed tab, clickingAdd to batch on each suggestion you want, and clickingCommit once you're done.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
development-tools/warnings.rst Outdated
| 1. Unexpected Warnings | ||
| a. Attempt to refactor the code to avoid the warning. | ||
| b. If it is not possible to avoid the warning document in the PR why it is | ||
| reasonable to ignore and add the warning to the platform specific | ||
| warning ignore file. If the file exists in the warning ignore file | ||
| increment the count by the number of new introduced warnings. | ||
| 2. Unexpected Imrpovements (Less Warnings) | ||
| a. Document in the PR that the change reduces the number of compiler | ||
| warnings. Decrement the count in the platform specific warning | ||
| ignore file or remove the file if the count is now zero. |
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.
| 1. UnexpectedWarnings | |
| a. Attempt to refactor the code to avoid the warning. | |
| b. If it is not possible to avoid the warning document in the PR why it is | |
| reasonable to ignore and add the warning to the platformspecific | |
| warning ignore file. If the file exists in the warning ignore file | |
| increment the count by the number ofnew introduced warnings. | |
| 2. UnexpectedImrpovements (Less Warnings) | |
| a. Document in the PR that the change reduces the number of compiler | |
| warnings. Decrement the count in the platformspecific warning | |
| ignore file or remove the file if the count is now zero. | |
| * Unexpectedwarnings | |
| * Attempt to refactor the code to avoid the warning. | |
| * If it is not possible to avoid the warning, document in the PR why it is | |
| reasonable to ignore and add the warning to the platform-specific | |
| warning ignore file. If the file exists in the warning ignore file | |
| increment the count by the number ofnewly introduced warnings. | |
| * Unexpectedimprovements (less warnings) | |
| * Document in the PR that the change reduces the number of compiler | |
| warnings. Decrement the count in the platform-specific warning | |
| ignore file or remove the file if the count is now zero. |
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 wanted this to read more like a decision tree and it looked like the only way for me to create a numbered list in rst was to explicitly create one. I changed this a bulleted list as suggested for now because when we start getting concrete examples of how to address warnings I want to really expand on this section.
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 see. I usually follow the Google guidelines on this; use bullets unless it's a sequence of steps to be performed in order:https://developers.google.com/style/lists
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.
Probably what I want is bullets and then the second level is a sequence of steps, since that is the case here. The top level is a condition: you failed the test because of one of these conditions and then what follows are the steps to address 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.
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.
Here's some suggestions again that were missed. There are still some pending ones from yesterday.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
development-tools/warnings.rst Outdated
| 1. Unexpected Warnings | ||
| a. Attempt to refactor the code to avoid the warning. | ||
| b. If it is not possible to avoid the warning document in the PR why it is | ||
| reasonable to ignore and add the warning to the platform specific | ||
| warning ignore file. If the file exists in the warning ignore file | ||
| increment the count by the number of new introduced warnings. | ||
| 2. Unexpected Imrpovements (Less Warnings) | ||
| a. Document in the PR that the change reduces the number of compiler | ||
| warnings. Decrement the count in the platform specific warning | ||
| ignore file or remove the file if the count is now zero. |
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 see. I usually follow the Google guidelines on this; use bullets unless it's a sequence of steps to be performed in order:https://developers.google.com/style/lists
Uh oh!
There was an error while loading.Please reload this page.
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 left several comments, many of which either include or supersede@hugovk's comments. In those cases, committing my comments should be enough (committing Hugo's first might create conflicts).
development-tools/warnings.rst Outdated
| Tools for tracking compiler warnings | ||
| ==================================== | ||
| The compiler warning tracking tooling is intended to alert developers to new |
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.
| The compiler warning tracking tooling is intended to alert developersto new | |
| The compiler warning tracking tooling is intended to alert developersabout new |
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 to about
development-tools/warnings.rst Outdated
| - Ubuntu/Build and Test | ||
| - macOS/Build and Test |
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.
| - Ubuntu/Build andTest | |
| - macOS/Build andTest | |
| * Ubuntu/build andtest (:cpy-file:`.github/workflows/reusable-ubuntu.yml`) | |
| * macOS/build andtest (:cpy-file:`.github/workflows/reusable-macos.yml`) |
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.
Included links to the configuration files
development-tools/warnings.rst Outdated
| compiler warnings introduced by their contributions. The tooling consists of | ||
| Python script which is ran by a few GitHub Actions workflows. These | ||
| GitHub Actions jobs run the warning checking tooling: |
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.
| compiler warnings introduced by their contributions. The tooling consists of | |
| Python script which is ran by a few GitHub Actions workflows. These | |
| GitHub Actions jobs run the warning checking tooling: | |
| compiler warnings introduced by their contributions. The tooling consists of | |
| a Python script which is ran by the following GitHub workflows: |
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.
Applied suggestion to make this more concise.
development-tools/warnings.rst Outdated
| ==================================== | ||
| The compiler warning tracking tooling is intended to alert developers to new |
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.
| ==================================== | |
| The compiler warning tracking tooling is intended to alert developers to new | |
| ==================================== | |
| ..highlight::bash | |
| The compiler warning tracking tooling is intended to alert developers to new |
We can use this to set the default highlight of the file.
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.
Sure, though we only two code-blocks with different highlights :)
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.
Added the default anyway!
development-tools/warnings.rst Outdated
| The help text for the:cpy-file:`Tools/build/check_warnings.py` is as follows: | ||
| ..code-block::text | ||
| usage: check_warnings.py [-h] | ||
| -c COMPILER_OUTPUT_FILE_PATH | ||
| [-i WARNING_IGNORE_FILE_PATH] [-x] [-X] -t {json,clang} | ||
| options: | ||
| -h, --help show this help message and exit | ||
| -c COMPILER_OUTPUT_FILE_PATH, --compiler-output-file-path COMPILER_OUTPUT_FILE_PATH | ||
| Path to the compiler output file | ||
| -i WARNING_IGNORE_FILE_PATH, --warning-ignore-file-path WARNING_IGNORE_FILE_PATH | ||
| Path to the warning ignore file | ||
| -x, --fail-on-regression | ||
| Flag to fail if new warnings are found | ||
| -X, --fail-on-improvement | ||
| Flag to fail if files that were expected to have warnings have no warnings | ||
| -t {json,clang}, --compiler-output-type {json,clang} | ||
| Type of compiler output file (json or clang) |
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.
| The help text for the:cpy-file:`Tools/build/check_warnings.py` is as follows: | |
| ..code-block::text | |
| usage: check_warnings.py [-h] | |
| -c COMPILER_OUTPUT_FILE_PATH | |
| [-i WARNING_IGNORE_FILE_PATH] [-x] [-X] -t {json,clang} | |
| options: | |
| -h, --help show this help message and exit | |
| -c COMPILER_OUTPUT_FILE_PATH, --compiler-output-file-path COMPILER_OUTPUT_FILE_PATH | |
| Path to the compiler output file | |
| -i WARNING_IGNORE_FILE_PATH, --warning-ignore-file-path WARNING_IGNORE_FILE_PATH | |
| Path to the warning ignore file | |
| -x, --fail-on-regression | |
| Flag to fail if new warnings are found | |
| -X, --fail-on-improvement | |
| Flag to fail if files that were expected to have warnings have no warnings | |
| -t {json,clang}, --compiler-output-type {json,clang} | |
| Type of compiler output file (json or clang) | |
| You can check the documentation for the:cpy-file:`Tools/build/check_warnings.py` tool | |
| by running:: | |
| python Tools/build/check_warnings.py --help |
I'm not sure it's a good idea duplicating the help text here, since it might become obsolete if new options are added. If people are going to use the tool, they might as well just use--help directly to see the latest version of the help.
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.
Probably a good idea. I figured this was a quick way for the reader to get an intuition of what inputs are going to the script if their only exposure to the tooling is that a CI failure has sent them here. I removed it to future-proof this guide.
development-tools/warnings.rst Outdated
| The warning check tooling will fail if the compiler generates more or less | ||
| warnings than expected for a given source file as defined in the | ||
| platform-specific warning ignore file. The warning ignore file is either | ||
| :cpy-file:`Tools/build/.warningignore_ubuntu` or:cpy-file:`Tools/build/.warningignore_macos` | ||
| depending on the platform. |
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.
| Thewarning check toolingwill fail if the compiler generates more or less | |
| warnings than expected for a given source file as defined in the | |
| platform-specific warning ignore file. The warning ignore file is either | |
| :cpy-file:`Tools/build/.warningignore_ubuntu` or:cpy-file:`Tools/build/.warningignore_macos` | |
| depending on the platform. | |
| The:cpy-file:`!check_warnings.py` toolwill fail if the compiler generates | |
| more or lesswarnings than expected for a given source file as defined in the | |
| platform-specific warning ignore file. The warning ignore file is either | |
| :cpy-file:`Tools/build/.warningignore_ubuntu` or | |
| :cpy-file:`Tools/build/.warningignore_macos`depending on the platform. |
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.
Is
:cpy-fil:`!check_warnings.py`supposed to be shorthand to link to a file with the same basename already referenced in the document? When build the dev guide with the suggestion it would take me tocpython/check_warnings.py.
I put in the full path for now.
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.
The! tells Sphinx not to generate a link, since for brevity I only used the filename and not the full path, a link can't be generated. By still using:cpy-file: the filename is semantically marked and has the same style as the other filenames marked with the same role. The file is already linked above, so there is no need to repeat the link.
Note that an alternative would have been:cpy-file:`check_warnings.py <Tools/build/check_warnings.py>`, which would have linked to the correct path while only showing the filename, but that doesn't seem to be supported by the:cpy-file: role.
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.
:cpy-file:`!check_warnings.py`Renders as
And actually links tocpython/check_warnings. Is that what you are suggesting?
development-tools/warnings.rst Outdated
| If a warning check fails with: | ||
| * Unexpected Warnings |
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.
| *Unexpected Warnings | |
| *More warnings than expected: |
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 kept the language the same but fixed the capitalization.
The reason being that the script actually labels warnings as "Unexpected improvements" and "Unexpected warnings"
development-tools/warnings.rst Outdated
| * If it is not possible to avoid the warning document in the PR why it is | ||
| reasonable to ignore and add the warning to the platform specific | ||
| warning ignore file. If the file exists in the warning ignore file | ||
| increment the count by the number of new introduced warnings. |
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.
| * If it is not possible to avoid the warning document in the PR why it is | |
| reasonable to ignore and add the warning to the platformspecific | |
| warningignore file. If the file exists in the warning ignore file | |
| increment the count by the number ofnew introduced warnings. | |
| * If it is not possible to avoid the warning document in the PR why it is | |
| reasonable to ignore and add the warning to the platform-specific warning | |
| ignore file. If the file already exists in the warning ignore file | |
| increment the count by the number ofnewly introduced warnings. |
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.
Fixed spacing
development-tools/warnings.rst Outdated
| reasonable to ignore and add the warning to the platform specific | ||
| warning ignore file. If the file exists in the warning ignore file | ||
| increment the count by the number of new introduced warnings. | ||
| * Unexpected Imrpovements (Less Warnings) |
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.
| *Unexpected Imrpovements (Less Warnings) | |
| *Less warnings than expected |
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 kept the language the way it was but fixed the capitalization, same reason as above the script actually labels specific warnings as such.
development-tools/warnings.rst Outdated
| * Document in the PR that the change reduces the number of compiler | ||
| warnings. Decrement the count in the platform specific warning | ||
| ignore file or remove the file if the count is now zero. |
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.
| * Document in the PR that the change reduces the number of compiler | |
| warnings. Decrement the count in the platformspecific warning | |
| ignore file or remove the file if the count is now zero. | |
| * Document in the PR that the change reduces the number of compiler | |
| warnings. Decrement the count in the platform-specific warning | |
| ignore file or remove the file if the count is now zero. |
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.
Fixed spacing
Uh oh!
There was an error while loading.Please reload this page.
I believe all of the review comments have been responded to (except for a clarification, if needed, from@ezio-melotti onthis point). If we are happy with this I believe this is blocking being able to link to this devguide from an error in the tooling to make it more clear to core developers what to do. Thanks all for the reviews, it is much appreciated! 💜 |
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
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 applied@ezio-melotti's suggestions. Let's merge, thanks!
Uh oh!
There was an error while loading.Please reload this page.
New warning tracking tooling was introduced to CPython GitHub Actions jobs for Ubuntu withpython/cpython#121730.
It was then added for macOS build and test jobs withpython/cpython#122211
New compiler options that will emit warnings for potential security issues are going to be introduced to CPython. This addition to the developers guide is intended to be a reference for developers if the warning tracking tooling fails GitHub Actions CI.
I intend further expand on this documentation with examples based on the warning flags that we introduce to CPython, how developers can attempt to refactor to avoid these warnings, and finer details on how to modify warning ignore files.
📚 Documentation preview 📚:https://cpython-devguide--1364.org.readthedocs.build/