From 58be1c270f2275603e56127791fa6777476954ec Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 6 Aug 2024 12:26:37 -0500 Subject: [PATCH] gh-112301: Add macOS warning tracking tooling (#122211) Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> --- .github/workflows/reusable-macos.yml | 4 +- .github/workflows/reusable-ubuntu.yml | 2 +- ...-07-24-05-18-25.gh-issue-112301.lfINgZ.rst | 2 + Tools/build/.warningignore_macos | 3 + Tools/build/check_warnings.py | 159 ++++++++++++------ 5 files changed, 116 insertions(+), 54 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst create mode 100644 Tools/build/.warningignore_macos diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 64ef2c91329..d77723ef27c 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -48,8 +48,10 @@ jobs: --prefix=/opt/python-dev \ --with-openssl="$(brew --prefix openssl@3.0)" - 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 diff --git a/.github/workflows/reusable-ubuntu.yml b/.github/workflows/reusable-ubuntu.yml index 8dd5f559585..92069fddc31 100644 --- a/.github/workflows/reusable-ubuntu.yml +++ b/.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 diff --git a/Misc/NEWS.d/next/Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst b/Misc/NEWS.d/next/Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst new file mode 100644 index 00000000000..81237e735eb --- /dev/null +++ b/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. diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos new file mode 100644 index 00000000000..1b504dfc540 --- /dev/null +++ b/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. diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index af9f7f169ad..31258932dbd 100644 --- a/Tools/build/check_warnings.py +++ b/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 clang + """ + # Regex to find warnings in the compiler output + clang_warning_regex = re.compile( + r"(?P.*):(?P\d+):(?P\d+): warning: (?P.*)" + ) + 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"), + } + ) + + 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 + -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. + 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("#") - } + 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 + ) - warnings = extract_warnings_from_compiler_output( - 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