gh-112301: Add macOS warning tracking tooling (#122211)

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>

Author: nohlson

.github/workflows/reusable-macos.yml

@@ -48,8 +48,10 @@ jobs:
           --prefix=/opt/python-dev \
           --with-openssl="$(brew --prefix [email protected])"
     - name: Build CPython
-      run: make -j8
+      run: set -o pipefail; make -j8 2>&1 | tee compiler_output.txt
     - name: Display build info
       run: make pythoninfo
+    - name: Check compiler warnings
+      run: python3 Tools/build/check_warnings.py --compiler-output-file-path=compiler_output.txt --warning-ignore-file-path=Tools/build/.warningignore_macos --compiler-output-type=clang
     - name: Tests
       run: make test

.github/workflows/reusable-ubuntu.yml

@@ -80,7 +80,7 @@ jobs:
       working-directory: ${{ env.CPYTHON_BUILDDIR }}
       run: make pythoninfo
     - name: Check compiler warnings
-      run: python Tools/build/check_warnings.py --compiler-output-file-path=${{ env.CPYTHON_BUILDDIR }}/compiler_output.txt --warning-ignore-file-path ${GITHUB_WORKSPACE}/Tools/build/.warningignore_ubuntu
+      run: python Tools/build/check_warnings.py --compiler-output-file-path=${{ env.CPYTHON_BUILDDIR }}/compiler_output.txt --warning-ignore-file-path ${GITHUB_WORKSPACE}/Tools/build/.warningignore_ubuntu --compiler-output-type=json
     - name: Remount sources writable for tests
       # some tests write to srcdir, lack of pyc files slows down testing
       run: sudo mount $CPYTHON_RO_SRCDIR -oremount,rw

Misc/NEWS.d/next/Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst

@@ -0,0 +1,2 @@
+Add macOS warning tracking to warning check tooling.
+Patch by Nate Ohlson.

Tools/build/.warningignore_macos

@@ -0,0 +1,3 @@
+# Files listed will be ignored by the compiler warning checker
+# for the macOS/build and test job.
+# Keep lines sorted lexicographically to help avoid merge conflicts.

Tools/build/check_warnings.py

@@ -2,39 +2,87 @@
 Parses compiler output with -fdiagnostics-format=json and checks that warnings
 exist only in files that are expected to have warnings.
 """
+
 import argparse
+from collections import defaultdict
 import json
 import re
 import sys
 from pathlib import Path
 
 
-def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]:
+def extract_warnings_from_compiler_output_clang(
+    compiler_output: str,
+) -> list[dict]:
     """
-    Extracts warnings from the compiler output when using
-    -fdiagnostics-format=json
+    Extracts warnings from the compiler output when using clang
+    """
+    # Regex to find warnings in the compiler output
+    clang_warning_regex = re.compile(
+        r"(?P<file>.*):(?P<line>\d+):(?P<column>\d+): warning: (?P<message>.*)"
+    )
+    compiler_warnings = []
+    for line in compiler_output.splitlines():
+        if match := clang_warning_regex.match(line):
+            compiler_warnings.append(
+                {
+                    "file": match.group("file"),
+                    "line": match.group("line"),
+                    "column": match.group("column"),
+                    "message": match.group("message"),
+                }
+            )
 
-    Compiler output as a whole is not a valid json document, but includes many
-    json objects and may include other output that is not json.
+    return compiler_warnings
+
+
+def extract_warnings_from_compiler_output_json(
+    compiler_output: str,
+) -> list[dict]:
     """
+    Extracts warnings from the compiler output when using
+    -fdiagnostics-format=json.
 
+    Compiler output as a whole is not a valid json document,
+    but includes many json objects and may include other output
+    that is not json.
+    """
     # Regex to find json arrays at the top level of the file
     # in the compiler output
     json_arrays = re.findall(
-        r"\[(?:[^\[\]]|\[(?:[^\[\]]|\[[^\[\]]*\])*\])*\]", compiler_output
+        r"\[(?:[^[\]]|\[[^\]]*\])*\]", compiler_output
     )
     compiler_warnings = []
     for array in json_arrays:
         try:
             json_data = json.loads(array)
             json_objects_in_array = [entry for entry in json_data]
-            compiler_warnings.extend(
-                [
-                    entry
-                    for entry in json_objects_in_array
-                    if entry.get("kind") == "warning"
-                ]
-            )
+            warning_list = [
+                entry
+                for entry in json_objects_in_array
+                if entry.get("kind") == "warning"
+            ]
+            for warning in warning_list:
+                locations = warning["locations"]
+                for location in locations:
+                    for key in ["caret", "start", "end"]:
+                        if key in location:
+                            compiler_warnings.append(
+                                {
+                                    # Remove leading current directory if present
+                                    "file": location[key]["file"].lstrip("./"),
+                                    "line": location[key]["line"],
+                                    "column": location[key]["column"],
+                                    "message": warning["message"],
+                                }
+                            )
+                            # Found a caret, start, or end in location so
+                            # break out completely to address next warning
+                            break
+                    else:
+                        continue
+                    break
+
         except json.JSONDecodeError:
             continue  # Skip malformed JSON
 
@@ -46,27 +94,16 @@ def get_warnings_by_file(warnings: list[dict]) -> dict[str, list[dict]]:
     Returns a dictionary where the key is the file and the data is the warnings
     in that file
     """
-    warnings_by_file = {}
+    warnings_by_file = defaultdict(list)
     for warning in warnings:
-        locations = warning["locations"]
-        for location in locations:
-            for key in ["caret", "start", "end"]:
-                if key in location:
-                    file = location[key]["file"]
-                    file = file.lstrip(
-                        "./"
-                    )  # Remove leading current directory if present
-                    if file not in warnings_by_file:
-                        warnings_by_file[file] = []
-                    warnings_by_file[file].append(warning)
+        warnings_by_file[warning["file"]].append(warning)
 
     return warnings_by_file
 
 
 def get_unexpected_warnings(
-    warnings: list[dict],
     files_with_expected_warnings: set[str],
-    files_with_warnings: set[str],
+    files_with_warnings: dict[str, list[dict]],
 ) -> int:
     """
     Returns failure status if warnings discovered in list of warnings
@@ -88,13 +125,12 @@ def get_unexpected_warnings(
 
 
 def get_unexpected_improvements(
-    warnings: list[dict],
     files_with_expected_warnings: set[str],
-    files_with_warnings: set[str],
+    files_with_warnings: dict[str, list[dict]],
 ) -> int:
     """
-    Returns failure status if there are no warnings in the list of warnings for
-    a file that is in the list of files with expected warnings
+    Returns failure status if there are no warnings in the list of warnings
+    for a file that is in the list of files with expected warnings
     """
     unexpected_improvements = []
     for file in files_with_expected_warnings:
@@ -123,7 +159,6 @@ def main(argv: list[str] | None = None) -> int:
         "-i",
         "--warning-ignore-file-path",
         type=str,
-        required=True,
         help="Path to the warning ignore file",
     )
     parser.add_argument(
@@ -141,6 +176,14 @@ def main(argv: list[str] | None = None) -> int:
         help="Flag to fail if files that were expected "
         "to have warnings have no warnings",
     )
+    parser.add_argument(
+        "-t",
+        "--compiler-output-type",
+        type=str,
+        required=True,
+        choices=["json", "clang"],
+        help="Type of compiler output file (json or clang)",
+    )
 
     args = parser.parse_args(argv)
 
@@ -149,44 +192,56 @@ def main(argv: list[str] | None = None) -> int:
     # Check that the compiler output file is a valid path
     if not Path(args.compiler_output_file_path).is_file():
         print(
-            "Compiler output file does not exist: "
-            f"{args.compiler_output_file_path}"
+            f"Compiler output file does not exist:"
+            f" {args.compiler_output_file_path}"
         )
         return 1
 
-    # Check that the warning ignore file is a valid path
-    if not Path(args.warning_ignore_file_path).is_file():
+    # Check that a warning ignore file was specified and if so is a valid path
+    if not args.warning_ignore_file_path:
         print(
-            "Warning ignore file does not exist: "
-            f"{args.warning_ignore_file_path}"
+            "Warning ignore file not specified."
+            " Continuing without it (no warnings ignored)."
         )
-        return 1
+        files_with_expected_warnings = set()
+    else:
+        if not Path(args.warning_ignore_file_path).is_file():
+            print(
+                f"Warning ignore file does not exist:"
+                f" {args.warning_ignore_file_path}"
+            )
+            return 1
+        with Path(args.warning_ignore_file_path).open(
+            encoding="UTF-8"
+        ) as clean_files:
+            files_with_expected_warnings = {
+                file.strip()
+                for file in clean_files
+                if file.strip() and not file.startswith("#")
+            }
 
     with Path(args.compiler_output_file_path).open(encoding="UTF-8") as f:
         compiler_output_file_contents = f.read()
 
-    with Path(args.warning_ignore_file_path).open(
-        encoding="UTF-8"
-    ) as clean_files:
-        files_with_expected_warnings = {
-            file.strip()
-            for file in clean_files
-            if file.strip() and not file.startswith("#")
-        }
-
-    warnings = extract_warnings_from_compiler_output(
-        compiler_output_file_contents
-    )
+    if args.compiler_output_type == "json":
+        warnings = extract_warnings_from_compiler_output_json(
+            compiler_output_file_contents
+        )
+    elif args.compiler_output_type == "clang":
+        warnings = extract_warnings_from_compiler_output_clang(
+            compiler_output_file_contents
+        )
+
     files_with_warnings = get_warnings_by_file(warnings)
 
     status = get_unexpected_warnings(
-        warnings, files_with_expected_warnings, files_with_warnings
+        files_with_expected_warnings, files_with_warnings
     )
     if args.fail_on_regression:
         exit_code |= status
 
     status = get_unexpected_improvements(
-        warnings, files_with_expected_warnings, files_with_warnings
+        files_with_expected_warnings, files_with_warnings
     )
     if args.fail_on_improvement:
         exit_code |= status