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

Add compiler warning tracking tooling failure remediation guidance #1364

Merged
merged 7 commits into from
Aug 24, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
"advanced-tools/clang.rst": "development-tools/clang.rst",
"advanced-tools/coverity.rst": "development-tools/coverity.rst",
"advanced-tools/gdb.rst": "development-tools/gdb.rst",
"advanced-tools/warnings.rst": "development-tools/warnings.rst",
hugovk marked this conversation as resolved.
Show resolved Hide resolved
# Core Developers
"coredev.rst": "core-developers/become-core-developer.rst",
"committing.rst": "core-developers/committing.rst",
Expand Down
1 change: 1 addition & 0 deletions development-tools/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ Development tools
gdb
clang
coverity
warnings
69 changes: 69 additions & 0 deletions development-tools/warnings.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
.. warnings:

Tools for tracking compiler warnings
====================================

The compiler warning tracking tooling is intended to alert developers to new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The compiler warning tracking tooling is intended to alert developers to new
The compiler warning tracking tooling is intended to alert developers about new

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to about

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
====================================
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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the default anyway!

compiler warnings introduced by their contributions. The tooling consists of
Python script which is ran by a few GitHub Actions workflows. These
hugovk marked this conversation as resolved.
Show resolved Hide resolved
GitHub Actions jobs run the warning checking tooling:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

Copy link
Contributor Author

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.


- Ubuntu/Build and Test
- macOS/Build and Test
hugovk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Ubuntu/Build and Test
- macOS/Build and Test
* Ubuntu/build and test (:cpy-file:`.github/workflows/reusable-ubuntu.yml`)
* macOS/build and test (:cpy-file:`.github/workflows/reusable-macos.yml`)

Copy link
Contributor Author

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


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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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.


The script can be run locally by providing the compiler output and the
compiler output type to see a list of unique warnings.

::
hugovk marked this conversation as resolved.
Show resolved Hide resolved

$ python Tools/build/check_warnings.py -c <compiler_output_file> -t <compiler_output_type>
hugovk marked this conversation as resolved.
Show resolved Hide resolved
hugovk marked this conversation as resolved.
Show resolved Hide resolved

..

Note: `-fdiagnostics-format=json` flag is required when compiling with GCC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Note: `-fdiagnostics-format=json` flag is required when compiling with GCC
Note: ``-fdiagnostics-format=json`` flag is required when compiling with GCC

for the script to properly parse the compiler output.
hugovk marked this conversation as resolved.
Show resolved Hide resolved

.. _warning-check-failure:

What to do if a warning check fails GitHub CI
---------------------------------------------

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
The :cpy-file:`!check_warnings.py` tool 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.

Copy link
Contributor Author

@nohlson nohlson Aug 8, 2024

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 to cpython/check_warnings.py.

I put in the full path for now.

Copy link
Member

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.

Copy link
Contributor Author

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

Screenshot 2024-08-10 at 7 34 43 PM

And actually links to cpython/check_warnings. Is that what you are suggesting?


If a warning check fails with:

* Unexpected Warnings
hugovk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Unexpected Warnings
* More warnings than expected:

Copy link
Contributor Author

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"

* 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
hugovk marked this conversation as resolved.
Show resolved Hide resolved
warning ignore file. If the file exists in the warning ignore file
increment the count by the number of new introduced warnings.
hugovk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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.
* 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 of newly introduced warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed spacing

* Unexpected Imrpovements (Less Warnings)
hugovk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Unexpected Imrpovements (Less Warnings)
* Less warnings than expected

Copy link
Contributor Author

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.

* Document in the PR that the change reduces the number of compiler
warnings. Decrement the count in the platform specific warning
hugovk marked this conversation as resolved.
Show resolved Hide resolved
ignore file or remove the file if the count is now zero.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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.
* 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed spacing

Loading