Add compiler warning tracking tooling failure remediation guidance #1364

nohlson · 2024-08-07T00:43:14Z

New warning tracking tooling was introduced to CPython GitHub Actions jobs for Ubuntu with python/cpython#121730.

It was then added for macOS build and test jobs with python/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/

…k failures

hugovk

Thanks! Some suggestions :)

Tip: You can apply all desired suggestions with one click by going to the Files changed tab, clicking Add to batch on each suggestion you want, and clicking Commit once you're done.

conf.py

development-tools/warnings.rst

hugovk · 2024-08-07T09:00:42Z

development-tools/warnings.rst

+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.


Suggested change

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.

* 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

warning ignore file. If the file exists in the warning ignore file

increment the count by the number of newly introduced warnings.

* Unexpected improvements (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.

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.

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

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.

development-tools/warnings.rst

hugovk

Here's some suggestions again that were missed. There are still some pending ones from yesterday.

conf.py

development-tools/warnings.rst

hugovk · 2024-08-08T12:56:57Z

development-tools/warnings.rst

+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.


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

development-tools/warnings.rst

ezio-melotti

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

ezio-melotti · 2024-08-08T13:12:11Z

development-tools/warnings.rst

+Tools for tracking compiler warnings
+====================================
+
+The compiler warning tracking tooling is intended to alert developers to new


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

changed to about

ezio-melotti · 2024-08-08T13:22:17Z

development-tools/warnings.rst

+- Ubuntu/Build and Test
+- macOS/Build and Test


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

Included links to the configuration files

ezio-melotti · 2024-08-08T13:23:24Z

development-tools/warnings.rst

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


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:

Applied suggestion to make this more concise.

ezio-melotti · 2024-08-08T13:28:10Z

development-tools/warnings.rst

+====================================
+
+The compiler warning tracking tooling is intended to alert developers to new


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.

Sure, though we only two code-blocks with different highlights :)

Added the default anyway!

ezio-melotti · 2024-08-08T13:30:35Z

development-tools/warnings.rst

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


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.

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.

ezio-melotti · 2024-08-08T13:45:59Z

development-tools/warnings.rst

+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.


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.

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.

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.

:cpy-file:`!check_warnings.py`

Renders as

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

ezio-melotti · 2024-08-08T13:50:05Z

development-tools/warnings.rst

+
+If a warning check fails with:
+
+* Unexpected Warnings


Suggested change

* Unexpected Warnings

* More warnings than expected:

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"

ezio-melotti · 2024-08-08T13:50:57Z

development-tools/warnings.rst

+    * 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.


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.

Fixed spacing

ezio-melotti · 2024-08-08T13:51:20Z

development-tools/warnings.rst

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


Suggested change

* Unexpected Imrpovements (Less Warnings)

* Less warnings than expected

I kept the language the way it was but fixed the capitalization, same reason as above the script actually labels specific warnings as such.

ezio-melotti · 2024-08-08T13:52:10Z

development-tools/warnings.rst

+    * 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.


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.

Fixed spacing

development-tools/warnings.rst

sethmlarson · 2024-08-20T20:16:20Z

I believe all of the review comments have been responded to (except for a clarification, if needed, from @ezio-melotti on this 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 <[email protected]>

hugovk

I applied @ezio-melotti's suggestions. Let's merge, thanks!

Add guidance for using warning tracking and remediate CI warning chec…

780b372

…k failures

hugovk reviewed Aug 7, 2024

View reviewed changes

nohlson added 2 commits August 7, 2024 14:20

Add links to files, general formatting improvements

3a6d880

Expand divider

db3ed48

hugovk changed the title ~~Add Compiler Warning Tracking Tooling Failure Remediation Guidance~~ Add compiler warning tracking tooling failure remediation guidance Aug 8, 2024

hugovk reviewed Aug 8, 2024

View reviewed changes

ezio-melotti reviewed Aug 8, 2024

View reviewed changes

nohlson added 2 commits August 8, 2024 15:01

Add formatting updates based on suggestions

cf4d962

Fix capitalization

f6395aa

ezio-melotti reviewed Aug 9, 2024

View reviewed changes

development-tools/warnings.rst Outdated Show resolved Hide resolved

Add newline

3aeb0c9

nohlson mentioned this pull request Aug 14, 2024

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

Merged

sethmlarson requested review from hugovk, AA-Turner and ezio-melotti August 20, 2024 20:16

Apply suggestions from code review

6ca97f8

Co-authored-by: Ezio Melotti <[email protected]>

hugovk approved these changes Aug 24, 2024

View reviewed changes

hugovk merged commit 94d4d60 into python:main Aug 24, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add compiler warning tracking tooling failure remediation guidance #1364

Add compiler warning tracking tooling failure remediation guidance #1364

nohlson commented Aug 7, 2024 •

edited by github-actions bot

Loading

hugovk left a comment

hugovk Aug 7, 2024

nohlson Aug 7, 2024

hugovk Aug 8, 2024

nohlson Aug 8, 2024

hugovk left a comment

hugovk Aug 8, 2024

ezio-melotti left a comment

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

hugovk Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024 •

edited

Loading

ezio-melotti Aug 9, 2024

nohlson Aug 11, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

ezio-melotti Aug 8, 2024

nohlson Aug 8, 2024

sethmlarson commented Aug 20, 2024

hugovk left a comment

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

		====================================

		The compiler warning tracking tooling is intended to alert developers to new

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

Add compiler warning tracking tooling failure remediation guidance #1364

Add compiler warning tracking tooling failure remediation guidance #1364

Conversation

nohlson commented Aug 7, 2024 • edited by github-actions bot Loading

hugovk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugovk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezio-melotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nohlson Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethmlarson commented Aug 20, 2024

hugovk left a comment

Choose a reason for hiding this comment

nohlson commented Aug 7, 2024 •

edited by github-actions bot

Loading

nohlson Aug 8, 2024 •

edited

Loading