Skip to content

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.

Sign up for GitHub

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

Merged
hugovk merged 7 commits into from
Aug 24, 2024
Merged

Add compiler warning tracking tooling failure remediation guidance #1364

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
780b372
Add guidance for using warning tracking and remediate CI warning chec…
nohlson Aug 7, 2024
3a6d880
Add links to files, general formatting improvements
nohlson Aug 7, 2024
db3ed48
Expand divider
nohlson Aug 7, 2024
cf4d962
Add formatting updates based on suggestions
nohlson Aug 8, 2024
f6395aa
Fix capitalization
nohlson Aug 8, 2024
3aeb0c9
Add newline
nohlson Aug 11, 2024
6ca97f8
Apply suggestions from code review
hugovk Aug 24, 2024
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
Prev Previous commit
Next Next commit
Add links to files, general formatting improvements
  • Loading branch information
@nohlson
nohlson committed Aug 7, 2024
commit 3a6d8802fe86780c04de58a84babd45b1a008649
1 change: 0 additions & 1 deletion conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
32 changes: 16 additions & 16 deletions development-tools/warnings.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

@ezio-melotti ezio-melotti Aug 8, 2024

Copy link
Copy Markdown
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

@nohlson nohlson Aug 8, 2024

Copy link
Copy Markdown
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

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
Comment thread
hugovk marked this conversation as resolved.
Outdated
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
Expand Down Expand Up @@ -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
Comment thread
hugovk marked this conversation as resolved.
Outdated

@ezio-melotti ezio-melotti Aug 8, 2024

Copy link
Copy Markdown
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:

@nohlson nohlson Aug 8, 2024

Copy link
Copy Markdown
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
Comment thread
hugovk marked this conversation as resolved.
Outdated
warning ignore file. If the file exists in the warning ignore file
increment the count by the number of new introduced warnings.
Comment thread
hugovk marked this conversation as resolved.
Outdated
2. Unexpected Imrpovements (Less Warnings)
a. Document in the PR that the change reduces the number of compiler
* Unexpected Imrpovements (Less Warnings)
Comment thread
hugovk marked this conversation as resolved.
Outdated

@ezio-melotti ezio-melotti Aug 8, 2024

Copy link
Copy Markdown
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

@nohlson nohlson Aug 8, 2024

Copy link
Copy Markdown
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
Comment thread
hugovk marked this conversation as resolved.
Outdated
ignore file or remove the file if the count is now zero.