From 780b372e41159dab3766f3e41e90bb4e8bcb780b Mon Sep 17 00:00:00 2001 From: mediumshade Date: Tue, 6 Aug 2024 19:35:21 -0500 Subject: [PATCH 1/7] Add guidance for using warning tracking and remediate CI warning check failures --- conf.py | 2 + development-tools/index.rst | 1 + development-tools/warnings.rst | 69 ++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 development-tools/warnings.rst diff --git a/conf.py b/conf.py index fc46f3223..80a043168 100644 --- a/conf.py +++ b/conf.py @@ -110,10 +110,12 @@ "clang.rst": "development-tools/clang.rst", "coverity.rst": "development-tools/coverity.rst", "gdb.rst": "development-tools/gdb.rst", + "warnings.rst": "development-tools/warnings.rst", # Advanced Tools was renamed Development Tools in gh-1149 "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", # Core Developers "coredev.rst": "core-developers/become-core-developer.rst", "committing.rst": "core-developers/committing.rst", diff --git a/development-tools/index.rst b/development-tools/index.rst index d7b88bf6d..d62a84769 100644 --- a/development-tools/index.rst +++ b/development-tools/index.rst @@ -9,3 +9,4 @@ Development tools gdb clang coverity + warnings diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst new file mode 100644 index 000000000..a98d5434c --- /dev/null +++ b/development-tools/warnings.rst @@ -0,0 +1,69 @@ +.. warnings: + +================================= +Compiler Warning Tracking Tooling +================================= + +The compiler warning tracking tooling is intended to alert developers to new +compiler warnings introduced by their contributions. The tooling consists of +python script which is ran by a few GitHub Actions worksflows. Currently these +GitHub Actions jobs run the warning checking tooling: + +- Ubuntu/Build and Test +- macOS/Build and Test + +The help text for the `Tools/build/check_warnings.py` is as follows: +:: + + 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) + +The script can be run locally by providing the compiler output and the +compiler output type to see a list of unique warnings. + +:: + + $ python Tools/build/check_warnings.py -c -t + +.. + + Note: `-fdiagnostics-format=json` flag is required when compiling with GCC + for the script to properly parse the compiler output. + +.. _warning-check-failure: + +What To Do If 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 +`Tools/build/.warningignore_ubuntu` or `Tools/build/.warningignore_macos` +depending on the platform. + +If warning check fails with: + +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. From 3a6d8802fe86780c04de58a84babd45b1a008649 Mon Sep 17 00:00:00 2001 From: mediumshade Date: Wed, 7 Aug 2024 14:20:27 -0500 Subject: [PATCH 2/7] Add links to files, general formatting improvements --- conf.py | 1 - development-tools/warnings.rst | 32 ++++++++++++++++---------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/conf.py b/conf.py index 80a043168..b62b7ea5f 100644 --- a/conf.py +++ b/conf.py @@ -110,7 +110,6 @@ "clang.rst": "development-tools/clang.rst", "coverity.rst": "development-tools/coverity.rst", "gdb.rst": "development-tools/gdb.rst", - "warnings.rst": "development-tools/warnings.rst", # Advanced Tools was renamed Development Tools in gh-1149 "advanced-tools/clang.rst": "development-tools/clang.rst", "advanced-tools/coverity.rst": "development-tools/coverity.rst", diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index a98d5434c..04ef2d7b2 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -1,19 +1,19 @@ .. warnings: -================================= -Compiler Warning Tracking Tooling -================================= +Tools for tracking compiler warnings +==================================== The compiler warning tracking tooling is intended to alert developers to new compiler warnings introduced by their contributions. The tooling consists of -python script which is ran by a few GitHub Actions worksflows. Currently these +Python script which is ran by a few GitHub Actions workflows. These GitHub Actions jobs run the warning checking tooling: - Ubuntu/Build and Test - macOS/Build and Test -The help text for the `Tools/build/check_warnings.py` is as follows: -:: +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 @@ -46,24 +46,24 @@ compiler output type to see a list of unique warnings. .. _warning-check-failure: -What To Do If Warning Check Fails GitHub CI +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 -`Tools/build/.warningignore_ubuntu` or `Tools/build/.warningignore_macos` +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. -If warning check fails with: +If a warning check fails with: -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 +* 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 new introduced warnings. -2. Unexpected Imrpovements (Less Warnings) - a. Document in the PR that the change reduces the number of compiler +* Unexpected Imrpovements (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. From db3ed481de81daf6c9c9b549015fd3d1e54d2532 Mon Sep 17 00:00:00 2001 From: mediumshade Date: Wed, 7 Aug 2024 14:25:41 -0500 Subject: [PATCH 3/7] Expand divider --- development-tools/warnings.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index 04ef2d7b2..9eec1bcda 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -47,7 +47,7 @@ compiler output type to see a list of unique warnings. .. _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 From cf4d9629c9e58002dd124964b4215baf2890187b Mon Sep 17 00:00:00 2001 From: mediumshade Date: Thu, 8 Aug 2024 15:01:29 -0500 Subject: [PATCH 4/7] Add formatting updates based on suggestions --- conf.py | 1 - development-tools/warnings.rst | 56 ++++++++++++---------------------- 2 files changed, 20 insertions(+), 37 deletions(-) diff --git a/conf.py b/conf.py index b62b7ea5f..fc46f3223 100644 --- a/conf.py +++ b/conf.py @@ -114,7 +114,6 @@ "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", # Core Developers "coredev.rst": "core-developers/become-core-developer.rst", "committing.rst": "core-developers/committing.rst", diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index 9eec1bcda..47caa596a 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -3,34 +3,18 @@ Tools for tracking compiler warnings ==================================== -The compiler warning tracking tooling is intended to alert developers to new -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: - -- Ubuntu/Build and Test -- macOS/Build and Test +.. highlight:: bash -The help text for the :cpy-file:`Tools/build/check_warnings.py` is as follows: - -.. code-block:: text +The compiler warning tracking tooling is intended to alert developers about new +compiler warnings introduced by their contributions. The tooling consists of +a Python script which is ran by the following GitHub workflows: - usage: check_warnings.py [-h] - -c COMPILER_OUTPUT_FILE_PATH - [-i WARNING_IGNORE_FILE_PATH] [-x] [-X] -t {json,clang} +* Ubuntu/build and test (:cpy-file:`.github/workflows/reusable-ubuntu.yml`) +* macOS/build and test (:cpy-file:`.github/workflows/reusable-macos.yml`) - 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 The script can be run locally by providing the compiler output and the compiler output type to see a list of unique warnings. @@ -49,21 +33,21 @@ compiler output type to see a list of unique warnings. 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 +The :cpy-file:`Tools/build/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. +:cpy-file:`Tools/build/.warningignore_ubuntu` or +:cpy-file:`Tools/build/.warningignore_macos` depending on the platform. If a warning check fails with: -* Unexpected Warnings +* 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 new introduced warnings. -* Unexpected Imrpovements (Less Warnings) + 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. + warnings. Decrement the count in the platform-specific warning + ignore file or remove the file if the count is now zero. From f6395aafa6beb826a7c3621cf2fe702139067cd4 Mon Sep 17 00:00:00 2001 From: mediumshade Date: Thu, 8 Aug 2024 15:04:13 -0500 Subject: [PATCH 5/7] Fix capitalization --- development-tools/warnings.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index 47caa596a..84a6dfcbd 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -47,7 +47,7 @@ If a warning check fails with: 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) +* 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. From 3aeb0c92aa08031950019aa06faae6aeb8a489a6 Mon Sep 17 00:00:00 2001 From: mediumshade Date: Sat, 10 Aug 2024 19:28:15 -0500 Subject: [PATCH 6/7] Add newline --- development-tools/warnings.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index 84a6dfcbd..5cec9ebe4 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -14,6 +14,7 @@ a Python script which is ran by the following GitHub workflows: You can check the documentation for the :cpy-file:`Tools/build/check_warnings.py` tool by running:: + python Tools/build/check_warnings.py --help The script can be run locally by providing the compiler output and the From 6ca97f88aef25399e4b94da483b5ba7be639d9be Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Sat, 24 Aug 2024 14:35:46 +0300 Subject: [PATCH 7/7] Apply suggestions from code review Co-authored-by: Ezio Melotti --- development-tools/warnings.rst | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/development-tools/warnings.rst b/development-tools/warnings.rst index 5cec9ebe4..dc4fb197e 100644 --- a/development-tools/warnings.rst +++ b/development-tools/warnings.rst @@ -17,17 +17,14 @@ by running:: python Tools/build/check_warnings.py --help -The script can be run locally by providing the compiler output and the -compiler output type to see a list of unique warnings. +The script can be run locally by providing the compiler output file +(where the output is saved) and the compiler output type +(either ``json`` or ``clang``) to see a list of unique warnings:: -:: + python Tools/build/check_warnings.py --compiler-output-file-path=compiler_output.txt --compiler-output-type=json - $ python Tools/build/check_warnings.py -c -t - -.. - - Note: `-fdiagnostics-format=json` flag is required when compiling with GCC - for the script to properly parse the compiler output. +.. note:: The ``-fdiagnostics-format=json`` flag is required when compiling with GCC + for the script to properly parse the compiler output. .. _warning-check-failure: