From 5148727e060c947a3fc0ebb942e4ce2dc2dad2e7 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 12:50:17 -0500 Subject: [PATCH 01/33] Create simple warning check tool and add to ubuntu build and test job --- .github/workflows/reusable-ubuntu.yml | 4 +- Tools/build/.warnignore_ubuntu | 3 + Tools/build/check_warnings.py | 181 ++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 Tools/build/.warnignore_ubuntu create mode 100644 Tools/build/check_warnings.py diff --git a/.github/workflows/reusable-ubuntu.yml b/.github/workflows/reusable-ubuntu.yml index 54d7765d159d49..3a6f2b910d0da0 100644 --- a/.github/workflows/reusable-ubuntu.yml +++ b/.github/workflows/reusable-ubuntu.yml @@ -74,10 +74,12 @@ jobs: ${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }} - name: Build CPython out-of-tree working-directory: ${{ env.CPYTHON_BUILDDIR }} - run: make -j4 + run: make -j4 &> compiler_output.txt - name: Display build info 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 - 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/Tools/build/.warnignore_ubuntu b/Tools/build/.warnignore_ubuntu new file mode 100644 index 00000000000000..8242c8d17c89fb --- /dev/null +++ b/Tools/build/.warnignore_ubuntu @@ -0,0 +1,3 @@ +# Files listed will be ignored by the compiler warning checker +# for the Ubuntu/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 new file mode 100644 index 00000000000000..b2119b40f3de6e --- /dev/null +++ b/Tools/build/check_warnings.py @@ -0,0 +1,181 @@ +#!/usr/bin/env python3 +import sys +from pathlib import Path +import argparse +import json + + +def extract_json_objects(compiler_output: str) -> list[dict]: + + return json_objects + + +def extract_warnings_from_compiler_output(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. + """ + # Extract JSON objects from the raw compiler output + compiler_output_json_objects = [] + stack = [] + start_index = None + for index, char in enumerate(compiler_output): + if char == '[': + if len(stack) == 0: + start_index = index # Start of a new JSON array + stack.append(char) + elif char == ']': + if len(stack) > 0: + stack.pop() + if len(stack) == 0 and start_index is not None: + try: + json_data = json.loads(compiler_output[start_index:index+1]) + compiler_output_json_objects.extend(json_data) + start_index = None + except json.JSONDecodeError: + continue # Skip malformed JSON + + compiler_warnings = [entry for entry in compiler_output_json_objects if entry.get('kind') == 'warning'] + + return compiler_warnings + + + +def get_unexpected_warnings( + warnings: list[dict], + files_with_expected_warnings: set[str], +) -> int: + """ + Fails if there are unexpected warnings + """ + unexpected_warnings = [] + for warning in warnings: + locations = warning['locations'] + for location in locations: + for key in ['caret', 'start', 'end']: + if key in location: + filename = location[key]['file'] + if filename not in files_with_expected_warnings: + unexpected_warnings.append(warning) + + if unexpected_warnings: + print("Unexpected warnings:") + for warning in unexpected_warnings: + print(warning) + return 1 + + + return 0 + + +def get_unexpected_improvements( + warnings: list[dict], + files_with_expected_warnings: set[str], +) -> int: + """ + Fails if files that were expected to have warnings have no warnings + """ + + # Create set of files with warnings + files_with_ewarnings = set() + for warning in warnings: + locations = warning['locations'] + for location in locations: + for key in ['caret', 'start', 'end']: + if key in location: + filename = location[key]['file'] + files_with_ewarnings.add(filename) + + + + unexpected_improvements = [] + for filename in files_with_expected_warnings: + if filename not in files_with_ewarnings: + unexpected_improvements.append(filename) + + if unexpected_improvements: + print("Unexpected improvements:") + for filename in unexpected_improvements: + print(filename) + return 1 + + + return 0 + + + + + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser() + parser.add_argument( + "--compiler-output-file-path", + type=str, + required=True, + help="Path to the file" + ) + parser.add_argument( + "--warning-ignore-file-path", + type=str, + required=True, + help="Path to the warning ignore file" + ) + + + args = parser.parse_args(argv) + + exit_code = 0 + + # Check that the compiler output file is a valid path + if not Path(args.compiler_output_file_path).is_file(): + print(f"Compiler output file does not exist: {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(): + print(f"Warning ignore file does not exist: {args.warning_ignore_file_path}") + return 1 + + 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 = { + filename.strip() + for filename in clean_files + if filename.strip() and not filename.startswith("#") + } + + + if len(compiler_output_file_contents) > 0: + print("Successfully got compiler output") + else: + exit_code = 1 + + if len(files_with_expected_warnings) > 0: + print("we have some exceptions") + + + warnings = extract_warnings_from_compiler_output(compiler_output_file_contents) + + exit_code = get_unexpected_warnings(warnings, files_with_expected_warnings) + + exit_code = get_unexpected_improvements(warnings, files_with_expected_warnings) + + + + + return exit_code + + + + + + + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file From 3bd5a10a9b57d3eafb94bc4a9a739f5834094593 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 13:18:25 -0500 Subject: [PATCH 02/33] Add flags to check warnings script to fail on regression or improvement --- Tools/build/check_warnings.py | 81 ++++++++++++++--------------------- 1 file changed, 33 insertions(+), 48 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index b2119b40f3de6e..daa60fb5282c9f 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -1,15 +1,13 @@ #!/usr/bin/env python3 +""" +Parses compiler output with -fdiagnostics-format=json and checks that warnings exist +only in files that are expected to have warnings. +""" import sys from pathlib import Path import argparse import json - -def extract_json_objects(compiler_output: str) -> list[dict]: - - return json_objects - - def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: """ Extracts warnings from the compiler output when using -fdiagnostics-format=json @@ -41,14 +39,13 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: return compiler_warnings - - def get_unexpected_warnings( warnings: list[dict], files_with_expected_warnings: set[str], ) -> int: """ - Fails if there are unexpected warnings + Returns failure status if warnings discovered in list of warnings are associated with a file + that is not found in the list of files with expected warnings """ unexpected_warnings = [] for warning in warnings: @@ -66,7 +63,6 @@ def get_unexpected_warnings( print(warning) return 1 - return 0 @@ -75,24 +71,23 @@ def get_unexpected_improvements( files_with_expected_warnings: set[str], ) -> int: """ - Fails if files that were expected to have warnings have no 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 """ # Create set of files with warnings - files_with_ewarnings = set() + files_with_warnings = set() for warning in warnings: locations = warning['locations'] for location in locations: for key in ['caret', 'start', 'end']: if key in location: filename = location[key]['file'] - files_with_ewarnings.add(filename) - - + files_with_warnings.add(filename) unexpected_improvements = [] for filename in files_with_expected_warnings: - if filename not in files_with_ewarnings: + if filename not in files_with_warnings: unexpected_improvements.append(filename) if unexpected_improvements: @@ -101,21 +96,15 @@ def get_unexpected_improvements( print(filename) return 1 - return 0 - - - - - def main(argv: list[str] | None = None) -> int: parser = argparse.ArgumentParser() parser.add_argument( "--compiler-output-file-path", type=str, required=True, - help="Path to the file" + help="Path to the compiler output file" ) parser.add_argument( "--warning-ignore-file-path", @@ -123,7 +112,18 @@ def main(argv: list[str] | None = None) -> int: required=True, help="Path to the warning ignore file" ) - + parser.add_argument( + "--fail-on-regression", + action="store_true", + default=False, + help="Flag to fail if new warnings are found" + ) + parser.add_argument( + "--fail-on-improvement", + action="store_true", + default=False, + help="Flag to fail if files that were expected to have warnings have no warnings" + ) args = parser.parse_args(argv) @@ -133,12 +133,12 @@ def main(argv: list[str] | None = None) -> int: if not Path(args.compiler_output_file_path).is_file(): print(f"Compiler output file does not exist: {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(): print(f"Warning ignore file does not exist: {args.warning_ignore_file_path}") return 1 - + with Path(args.compiler_output_file_path).open(encoding="UTF-8") as f: compiler_output_file_contents = f.read() @@ -149,33 +149,18 @@ def main(argv: list[str] | None = None) -> int: if filename.strip() and not filename.startswith("#") } - - if len(compiler_output_file_contents) > 0: - print("Successfully got compiler output") - else: - exit_code = 1 - - if len(files_with_expected_warnings) > 0: - print("we have some exceptions") - + if not len(compiler_output_file_contents) > 0:exit_code = 1 warnings = extract_warnings_from_compiler_output(compiler_output_file_contents) - exit_code = get_unexpected_warnings(warnings, files_with_expected_warnings) - - exit_code = get_unexpected_improvements(warnings, files_with_expected_warnings) - - + status = get_unexpected_warnings(warnings, files_with_expected_warnings) + if args.fail_on_regression: exit_code |= status + status = get_unexpected_improvements(warnings, files_with_expected_warnings) + if args.fail_on_improvement: exit_code |= status + print("Returning exit code: ", exit_code) return exit_code - - - - - - - if __name__ == "__main__": - sys.exit(main()) \ No newline at end of file + sys.exit(main()) From 68130035e6f1f9ca16af4defe38ff12d46f5eb28 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 13:34:59 -0500 Subject: [PATCH 03/33] Remove redundant comment --- Tools/build/check_warnings.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index daa60fb5282c9f..2e2c6b5b8a9d45 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -15,7 +15,6 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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. """ - # Extract JSON objects from the raw compiler output compiler_output_json_objects = [] stack = [] start_index = None From ab4d754775810ea8669868e00a30e5097e6c37ea Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 14:24:34 -0500 Subject: [PATCH 04/33] Rename warnigore file to warningignore --- Tools/build/{.warnignore_ubuntu => .warningignore_ubuntu} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename Tools/build/{.warnignore_ubuntu => .warningignore_ubuntu} (100%) diff --git a/Tools/build/.warnignore_ubuntu b/Tools/build/.warningignore_ubuntu similarity index 100% rename from Tools/build/.warnignore_ubuntu rename to Tools/build/.warningignore_ubuntu From 615d2282aa9baa5183acedb47bf950633b085486 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 16:13:45 -0500 Subject: [PATCH 05/33] Use regex to extract json arrays --- Tools/build/check_warnings.py | 63 +++++++++++++++-------------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 2e2c6b5b8a9d45..b655b11f707449 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -7,6 +7,7 @@ from pathlib import Path import argparse import json +import re def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: """ @@ -15,26 +16,17 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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_json_objects = [] - stack = [] - start_index = None - for index, char in enumerate(compiler_output): - if char == '[': - if len(stack) == 0: - start_index = index # Start of a new JSON array - stack.append(char) - elif char == ']': - if len(stack) > 0: - stack.pop() - if len(stack) == 0 and start_index is not None: - try: - json_data = json.loads(compiler_output[start_index:index+1]) - compiler_output_json_objects.extend(json_data) - start_index = None - except json.JSONDecodeError: - continue # Skip malformed JSON - - compiler_warnings = [entry for entry in compiler_output_json_objects if entry.get('kind') == 'warning'] + + # Regex to find json arrays at the top level of the file in the compiler output + json_arrays = re.findall(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']) + except json.JSONDecodeError: + continue # Skip malformed JSON return compiler_warnings @@ -52,8 +44,9 @@ def get_unexpected_warnings( for location in locations: for key in ['caret', 'start', 'end']: if key in location: - filename = location[key]['file'] - if filename not in files_with_expected_warnings: + file = location[key]['file'] + file = file.lstrip('./') # Remove leading curdir if present + if file not in files_with_expected_warnings: unexpected_warnings.append(warning) if unexpected_warnings: @@ -81,18 +74,19 @@ def get_unexpected_improvements( for location in locations: for key in ['caret', 'start', 'end']: if key in location: - filename = location[key]['file'] - files_with_warnings.add(filename) + file = location[key]['file'] + file = file.lstrip('./') # Remove leading curdir if present + files_with_warnings.add(file) unexpected_improvements = [] - for filename in files_with_expected_warnings: - if filename not in files_with_warnings: - unexpected_improvements.append(filename) + for file in files_with_expected_warnings: + if file not in files_with_warnings: + unexpected_improvements.append(file) if unexpected_improvements: print("Unexpected improvements:") - for filename in unexpected_improvements: - print(filename) + for file in unexpected_improvements: + print(file) return 1 return 0 @@ -143,13 +137,11 @@ def main(argv: list[str] | None = None) -> int: with Path(args.warning_ignore_file_path).open(encoding="UTF-8") as clean_files: files_with_expected_warnings = { - filename.strip() - for filename in clean_files - if filename.strip() and not filename.startswith("#") + file.strip() + for file in clean_files + if file.strip() and not file.startswith("#") } - if not len(compiler_output_file_contents) > 0:exit_code = 1 - warnings = extract_warnings_from_compiler_output(compiler_output_file_contents) status = get_unexpected_warnings(warnings, files_with_expected_warnings) @@ -157,8 +149,7 @@ def main(argv: list[str] | None = None) -> int: status = get_unexpected_improvements(warnings, files_with_expected_warnings) if args.fail_on_improvement: exit_code |= status - - print("Returning exit code: ", exit_code) + return exit_code if __name__ == "__main__": From fc0a60ba7e6a902b0abd17faa30b18c97f1ad90c Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 16:16:29 -0500 Subject: [PATCH 06/33] Trim whitespace --- Tools/build/check_warnings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index b655b11f707449..16075d855a6e39 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -149,7 +149,7 @@ def main(argv: list[str] | None = None) -> int: status = get_unexpected_improvements(warnings, files_with_expected_warnings) if args.fail_on_improvement: exit_code |= status - + return exit_code if __name__ == "__main__": From 7793d80836c9fa1adc7841acf25f4f135fdeaa99 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 16:30:09 -0500 Subject: [PATCH 07/33] Test on github unexpected improvement --- Tools/build/.warningignore_ubuntu | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tools/build/.warningignore_ubuntu b/Tools/build/.warningignore_ubuntu index 8242c8d17c89fb..83dccf0eb215f6 100644 --- a/Tools/build/.warningignore_ubuntu +++ b/Tools/build/.warningignore_ubuntu @@ -1,3 +1,5 @@ # Files listed will be ignored by the compiler warning checker # for the Ubuntu/build and test job. # Keep lines sorted lexicographically to help avoid merge conflicts. + +Modules/_decimal/libmpdec/memory.c \ No newline at end of file From 99715d235cc437e6b5ce7cb194e34559149296c2 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 16:36:42 -0500 Subject: [PATCH 08/33] Add config for improve fail check --- .github/workflows/reusable-ubuntu.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/reusable-ubuntu.yml b/.github/workflows/reusable-ubuntu.yml index 3a6f2b910d0da0..9b05e1141d357c 100644 --- a/.github/workflows/reusable-ubuntu.yml +++ b/.github/workflows/reusable-ubuntu.yml @@ -79,7 +79,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 --fail-on-improvement - 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 From bc44ec223af3ed552b0d769ffd5629f8a540ebf2 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 16:38:27 -0500 Subject: [PATCH 09/33] Revert to prod check warning state --- .github/workflows/reusable-ubuntu.yml | 2 +- Tools/build/.warningignore_ubuntu | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/reusable-ubuntu.yml b/.github/workflows/reusable-ubuntu.yml index 9b05e1141d357c..3a6f2b910d0da0 100644 --- a/.github/workflows/reusable-ubuntu.yml +++ b/.github/workflows/reusable-ubuntu.yml @@ -79,7 +79,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 --fail-on-improvement + 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 - 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/Tools/build/.warningignore_ubuntu b/Tools/build/.warningignore_ubuntu index 83dccf0eb215f6..8242c8d17c89fb 100644 --- a/Tools/build/.warningignore_ubuntu +++ b/Tools/build/.warningignore_ubuntu @@ -1,5 +1,3 @@ # Files listed will be ignored by the compiler warning checker # for the Ubuntu/build and test job. # Keep lines sorted lexicographically to help avoid merge conflicts. - -Modules/_decimal/libmpdec/memory.c \ No newline at end of file From e2ca75f3991e20b453622b9917fc6182938ed57b Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 13 Jul 2024 21:55:58 +0000 Subject: [PATCH 10/33] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst diff --git a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst new file mode 100644 index 00000000000000..27ccd8e63d5d9e --- /dev/null +++ b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst @@ -0,0 +1 @@ +Add tooling to check for changes in compiler warnings. From bd1634e3c3504b4b79423c9f65a50bbf30205241 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 16 Jul 2024 22:32:12 -0500 Subject: [PATCH 11/33] Refactor creating set of files with warnings to a dedicated function --- ...-07-13-21-55-58.gh-issue-112301.YJS1dl.rst | 1 + Tools/build/check_warnings.py | 61 ++++++++++--------- 2 files changed, 34 insertions(+), 28 deletions(-) diff --git a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst index 27ccd8e63d5d9e..4dd10fb746d44d 100644 --- a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst +++ b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst @@ -1 +1,2 @@ Add tooling to check for changes in compiler warnings. +Patch by Nate Ohlson \ No newline at end of file diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 16075d855a6e39..6c048ecd845dfa 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -3,11 +3,11 @@ Parses compiler output with -fdiagnostics-format=json and checks that warnings exist only in files that are expected to have warnings. """ -import sys -from pathlib import Path import argparse import json import re +import sys +from pathlib import Path def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: """ @@ -24,63 +24,67 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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']) + compiler_warnings.extend([entry for entry in json_objects_in_array + if entry.get('kind') == 'warning']) except json.JSONDecodeError: continue # Skip malformed JSON return compiler_warnings +# Create a function that returns a dictionary where the key is the file and the data is the warnings in that file +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 = {} + 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) + + return warnings_by_file + def get_unexpected_warnings( warnings: list[dict], files_with_expected_warnings: set[str], + files_with_warnings: set[str], ) -> int: """ Returns failure status if warnings discovered in list of warnings are associated with a file that is not found in the list of files with expected warnings """ unexpected_warnings = [] - 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 curdir if present - if file not in files_with_expected_warnings: - unexpected_warnings.append(warning) + for file in files_with_warnings.keys(): + if file not in files_with_expected_warnings: + unexpected_warnings.extend(files_with_warnings[file]) if unexpected_warnings: print("Unexpected warnings:") for warning in unexpected_warnings: print(warning) return 1 - + return 0 def get_unexpected_improvements( warnings: list[dict], files_with_expected_warnings: set[str], + files_with_warnings: set[str], ) -> 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 """ - - # Create set of files with warnings - files_with_warnings = set() - 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 curdir if present - files_with_warnings.add(file) - unexpected_improvements = [] for file in files_with_expected_warnings: - if file not in files_with_warnings: + if file not in files_with_warnings.keys(): unexpected_improvements.append(file) if unexpected_improvements: @@ -143,11 +147,12 @@ def main(argv: list[str] | None = None) -> int: } 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) + status = get_unexpected_warnings(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) + status = get_unexpected_improvements(warnings, files_with_expected_warnings, files_with_warnings) if args.fail_on_improvement: exit_code |= status return exit_code From b07b1d6241840639a5c2d90a31644e02d245b273 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 02:41:34 -0500 Subject: [PATCH 12/33] Move cflags configure option to top level build configuration --- Tools/build/check_warnings.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 6c048ecd845dfa..1330a14f91ba5d 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -69,9 +69,8 @@ def get_unexpected_warnings( for warning in unexpected_warnings: print(warning) return 1 - - return 0 + return 0 def get_unexpected_improvements( warnings: list[dict], From e1954a53dd2b25098ebeb5b301cdb3188a61a5aa Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 03:20:24 -0500 Subject: [PATCH 13/33] Add json diagnostics to ubuntu configuration as first argument --- .github/workflows/reusable-ubuntu.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/reusable-ubuntu.yml b/.github/workflows/reusable-ubuntu.yml index 3a6f2b910d0da0..c6289a74e9a5f6 100644 --- a/.github/workflows/reusable-ubuntu.yml +++ b/.github/workflows/reusable-ubuntu.yml @@ -67,6 +67,7 @@ jobs: working-directory: ${{ env.CPYTHON_BUILDDIR }} run: >- ../cpython-ro-srcdir/configure + CFLAGS="-fdiagnostics-format=json" --config-cache --with-pydebug --enable-slower-safety From 59351430878fb1195d5a1d309f2ae6f16c384c91 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 15:44:55 -0500 Subject: [PATCH 14/33] Add newline to news --- .../next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst index 4dd10fb746d44d..4e8a8b6dd36ba6 100644 --- a/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst +++ b/Misc/NEWS.d/next/Tests/2024-07-13-21-55-58.gh-issue-112301.YJS1dl.rst @@ -1,2 +1,2 @@ Add tooling to check for changes in compiler warnings. -Patch by Nate Ohlson \ No newline at end of file +Patch by Nate Ohlson From 7f1a238b3a26fc8fcee4629dcb62753f369b7fa2 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 12:50:17 -0500 Subject: [PATCH 15/33] Create simple warning check tool and add to ubuntu build and test job --- Tools/build/.warnignore_ubuntu | 3 +++ Tools/build/check_warnings.py | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) create mode 100644 Tools/build/.warnignore_ubuntu diff --git a/Tools/build/.warnignore_ubuntu b/Tools/build/.warnignore_ubuntu new file mode 100644 index 00000000000000..8242c8d17c89fb --- /dev/null +++ b/Tools/build/.warnignore_ubuntu @@ -0,0 +1,3 @@ +# Files listed will be ignored by the compiler warning checker +# for the Ubuntu/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 1330a14f91ba5d..712a505d057f4c 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -129,12 +129,10 @@ def main(argv: list[str] | None = None) -> int: if not Path(args.compiler_output_file_path).is_file(): print(f"Compiler output file does not exist: {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(): print(f"Warning ignore file does not exist: {args.warning_ignore_file_path}") return 1 - with Path(args.compiler_output_file_path).open(encoding="UTF-8") as f: compiler_output_file_contents = f.read() From 144136e8c7b3a4a8b846954e20f1be038bfbb52a Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 13:59:08 -0500 Subject: [PATCH 16/33] Add macos warning checks to GitHub actions --- .github/workflows/reusable-macos.yml | 10 +++++++--- Tools/build/.warningignore_macos | 3 +++ 2 files changed, 10 insertions(+), 3 deletions(-) create mode 100644 Tools/build/.warningignore_macos diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 0f189960dbea61..16e7bc71ff5dfd 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -26,13 +26,15 @@ jobs: strategy: fail-fast: false matrix: - os: ${{fromJson(inputs.os-matrix)}} + os: + - "macos-10.15" + - "macos-11" is-fork: - ${{ github.repository_owner != 'python' }} exclude: - os: "ghcr.io/cirruslabs/macos-runner:sonoma" is-fork: true - - os: "macos-14" + - os: "macos-10.15" is-fork: false runs-on: ${{ matrix.os }} steps: @@ -58,8 +60,10 @@ jobs: --prefix=/opt/python-dev \ --with-openssl="$(brew --prefix openssl@3.0)" - name: Build CPython - run: make -j8 + run: make -j8 &> compiler_output.txt - name: Display build info 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_macos - name: Tests run: make test diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos new file mode 100644 index 00000000000000..1b504dfc54000f --- /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. From 3dd40fd960d086ad41f57d1a03d5e83f63095dd6 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 14:32:34 -0500 Subject: [PATCH 17/33] Revert reusable-macos.yml for environment variables --- .github/workflows/reusable-macos.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 16e7bc71ff5dfd..e54de82a9ecf4a 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -26,15 +26,13 @@ jobs: strategy: fail-fast: false matrix: - os: - - "macos-10.15" - - "macos-11" + os: ${{fromJson(inputs.os-matrix)}} is-fork: - ${{ github.repository_owner != 'python' }} exclude: - os: "ghcr.io/cirruslabs/macos-runner:sonoma" is-fork: true - - os: "macos-10.15" + - os: "macos-14" is-fork: false runs-on: ${{ matrix.os }} steps: From 1158f53bc5e4976fc18fef9b460ede66c2bca3d2 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 14:46:16 -0500 Subject: [PATCH 18/33] Update paths --- .github/workflows/reusable-macos.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index e54de82a9ecf4a..0751f2f63a564b 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -62,6 +62,6 @@ jobs: - name: Display build info 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_macos + run: python Tools/build/check_warnings.py --compiler-output-file-path=compiler_output.txt --warning-ignore-file-path=Tools/build/.warningignore_macos - name: Tests run: make test From cb51b4f854cf2c8c65c5f65b6c24d89b3c17abeb Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 15:03:30 -0500 Subject: [PATCH 19/33] Test unexpected improvement --- Tools/build/.warningignore_macos | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos index 1b504dfc54000f..ec6dc2c54367e9 100644 --- a/Tools/build/.warningignore_macos +++ b/Tools/build/.warningignore_macos @@ -1,3 +1,5 @@ # 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. + +Modules/_abc.c \ No newline at end of file From 77e0f6e7850d3696fffaa4c08e73053a9f8922ef Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 16:08:53 -0500 Subject: [PATCH 20/33] Remove warning ignore --- Tools/build/.warningignore_macos | 2 -- 1 file changed, 2 deletions(-) diff --git a/Tools/build/.warningignore_macos b/Tools/build/.warningignore_macos index ec6dc2c54367e9..1b504dfc54000f 100644 --- a/Tools/build/.warningignore_macos +++ b/Tools/build/.warningignore_macos @@ -1,5 +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. - -Modules/_abc.c \ No newline at end of file From 3e1d75f14c87505055e4b5a9c269068253a29b76 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 22 Jul 2024 16:57:19 -0500 Subject: [PATCH 21/33] Add json output option to macos configure job --- .github/workflows/reusable-macos.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 0751f2f63a564b..6241b11fa6106f 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -51,6 +51,7 @@ jobs: GDBM_CFLAGS="-I$(brew --prefix gdbm)/include" \ GDBM_LIBS="-L$(brew --prefix gdbm)/lib -lgdbm" \ ./configure \ + CFLAGS="--fdiagnostics-format=json" \ --config-cache \ --with-pydebug \ --enable-slower-safety \ From b5cd58ae585710776f2d48410f2146e3a45fc065 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 00:09:14 -0500 Subject: [PATCH 22/33] Add common dictionary format when parsing warnings --- .github/workflows/reusable-macos.yml | 2 +- .github/workflows/reusable-ubuntu.yml | 2 +- Tools/build/check_warnings.py | 70 ++++++++++++++++++++------- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 6241b11fa6106f..9adda38cf165d2 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -63,6 +63,6 @@ jobs: - name: Display build info run: make pythoninfo - name: Check compiler warnings - run: python Tools/build/check_warnings.py --compiler-output-file-path=compiler_output.txt --warning-ignore-file-path=Tools/build/.warningignore_macos + run: python 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 c6289a74e9a5f6..670dec62c4ea30 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/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 712a505d057f4c..5c359a69462c9f 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -9,7 +9,27 @@ 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(): + match = clang_warning_regex.match(line) + if match: + 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 @@ -24,8 +44,20 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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({ + 'file': location[key]['file'].lstrip('./'), # Remove leading current directory if present + 'line': location[key]['line'], + 'column': location[key]['column'], + 'message': warning['message'], + }) + + except json.JSONDecodeError: continue # Skip malformed JSON @@ -38,20 +70,14 @@ def get_warnings_by_file(warnings: list[dict]) -> dict[str, list[dict]]: """ warnings_by_file = {} 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) + file = warning['file'] + if file not in warnings_by_file: + warnings_by_file[file] = [] + warnings_by_file[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], ) -> int: @@ -73,7 +99,6 @@ def get_unexpected_warnings( return 0 def get_unexpected_improvements( - warnings: list[dict], files_with_expected_warnings: set[str], files_with_warnings: set[str], ) -> int: @@ -120,6 +145,13 @@ def main(argv: list[str] | None = None) -> int: default=False, help="Flag to fail if files that were expected to have warnings have no warnings" ) + parser.add_argument( + "--compiler-output-type", + type=str, + required=True, + choices=["json", "clang"], + help="Type of compiler output file (json or clang)" + ) args = parser.parse_args(argv) @@ -143,13 +175,17 @@ def main(argv: list[str] | None = None) -> int: 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) + status = get_unexpected_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) + status = get_unexpected_improvements(files_with_expected_warnings, files_with_warnings) if args.fail_on_improvement: exit_code |= status return exit_code From 02f313e37bbf052fa1e7a09cb2c29d38abf9d095 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 00:11:42 -0500 Subject: [PATCH 23/33] Remove configure option for macos job --- .github/workflows/reusable-macos.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 9adda38cf165d2..530855212ba33e 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -51,7 +51,6 @@ jobs: GDBM_CFLAGS="-I$(brew --prefix gdbm)/include" \ GDBM_LIBS="-L$(brew --prefix gdbm)/lib -lgdbm" \ ./configure \ - CFLAGS="--fdiagnostics-format=json" \ --config-cache \ --with-pydebug \ --enable-slower-safety \ From 83d1ed7100f45d0d43bf823dc9087107b93a8272 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 01:55:22 -0500 Subject: [PATCH 24/33] Print out json version of compiler output --- Tools/build/check_warnings.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 5c359a69462c9f..c6061085a12bd3 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -36,6 +36,9 @@ def extract_warnings_from_compiler_output_json(compiler_output: str) -> list[dic 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. """ + print("######") + print(compiler_output) + print("&&&&&") # Regex to find json arrays at the top level of the file in the compiler output json_arrays = re.findall(r'\[(?:[^\[\]]|\[(?:[^\[\]]|\[[^\[\]]*\])*\])*\]', compiler_output) From 08a6f6d1d06ecf81fae7258adfebee164ae1f0a5 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 23 Jul 2024 16:22:50 -0500 Subject: [PATCH 25/33] Remove old version of warning ignore file --- Tools/build/.warnignore_ubuntu | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 Tools/build/.warnignore_ubuntu diff --git a/Tools/build/.warnignore_ubuntu b/Tools/build/.warnignore_ubuntu deleted file mode 100644 index 8242c8d17c89fb..00000000000000 --- a/Tools/build/.warnignore_ubuntu +++ /dev/null @@ -1,3 +0,0 @@ -# Files listed will be ignored by the compiler warning checker -# for the Ubuntu/build and test job. -# Keep lines sorted lexicographically to help avoid merge conflicts. From 7be8ee6b1ea327c161f7d3f268ac169d726add1b Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Wed, 24 Jul 2024 00:15:26 -0500 Subject: [PATCH 26/33] Remove compiler output print diagnostic --- Tools/build/check_warnings.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index c6061085a12bd3..f5e8ec8efdd943 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -13,7 +13,6 @@ def extract_warnings_from_compiler_output_clang(compiler_output: str) -> list[di """ 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 = [] @@ -36,10 +35,6 @@ def extract_warnings_from_compiler_output_json(compiler_output: str) -> list[dic 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. """ - print("######") - print(compiler_output) - print("&&&&&") - # Regex to find json arrays at the top level of the file in the compiler output json_arrays = re.findall(r'\[(?:[^\[\]]|\[(?:[^\[\]]|\[[^\[\]]*\])*\])*\]', compiler_output) compiler_warnings = [] From 8b0a2eeaec843dbb5ba5defcf43acd37cf28a0fa Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Wed, 24 Jul 2024 05:18:26 +0000 Subject: [PATCH 27/33] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20b?= =?UTF-8?q?lurb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst 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 00000000000000..bc9f6758108ce1 --- /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 From 49cbd87cc6c4c88bf5721c601e36e50bbb51cbae Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Wed, 24 Jul 2024 00:21:37 -0500 Subject: [PATCH 28/33] Remove superfluous comment --- Tools/build/check_warnings.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index f5e8ec8efdd943..9771977d6e705c 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -61,7 +61,6 @@ def extract_warnings_from_compiler_output_json(compiler_output: str) -> list[dic return compiler_warnings -# Create a function that returns a dictionary where the key is the file and the data is the warnings in that file 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 From b654a84c46debebe2edbb63f19d646a8ff0928d8 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 30 Jul 2024 10:26:34 -0500 Subject: [PATCH 29/33] Add period to news --- .../Security/2024-07-24-05-18-25.gh-issue-112301.lfINgZ.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index bc9f6758108ce1..81237e735ebdb7 100644 --- 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 @@ -1,2 +1,2 @@ -Add macOS warning tracking to warning check tooling. -Patch by Nate Ohlson +Add macOS warning tracking to warning check tooling. +Patch by Nate Ohlson. From 3688c5c2639d22b28c15a74b83ce912a775c2201 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 30 Jul 2024 22:20:43 -0500 Subject: [PATCH 30/33] Make warning ignore file optional --- Tools/build/check_warnings.py | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index 77347c752ef6e9..e936bc372f5cc1 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -151,7 +151,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( @@ -187,24 +186,31 @@ def main(argv: list[str] | None = None) -> int: f"Compiler output file does not exist: {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( - f"Warning ignore file does not exist: {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: {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 From cb1f276f5e30a8f05a4199a82afa64a8e56e2394 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Tue, 30 Jul 2024 22:49:35 -0500 Subject: [PATCH 31/33] Add write compiler output to log and file --- .github/workflows/reusable-macos.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/reusable-macos.yml b/.github/workflows/reusable-macos.yml index 8e3f7e52615478..d77723ef27c2dc 100644 --- a/.github/workflows/reusable-macos.yml +++ b/.github/workflows/reusable-macos.yml @@ -48,10 +48,10 @@ jobs: --prefix=/opt/python-dev \ --with-openssl="$(brew --prefix openssl@3.0)" - name: Build CPython - run: make -j8 &> compiler_output.txt + run: set -o pipefail; make -j8 2>&1 | tee compiler_output.txt - name: Display build info run: make pythoninfo - name: Check compiler warnings - run: python Tools/build/check_warnings.py --compiler-output-file-path=compiler_output.txt --warning-ignore-file-path=Tools/build/.warningignore_macos --compiler-output-type=clang + 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 From fb91e3e2d38aa89b2779e18f94645733915b01f1 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Wed, 31 Jul 2024 12:02:40 -0500 Subject: [PATCH 32/33] Fix formatting and update regex --- Tools/build/check_warnings.py | 47 +++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index e936bc372f5cc1..ddadde6583264b 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -2,7 +2,9 @@ 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 @@ -21,8 +23,7 @@ def extract_warnings_from_compiler_output_clang( ) compiler_warnings = [] for line in compiler_output.splitlines(): - match = clang_warning_regex.match(line) - if match: + if match := clang_warning_regex.match(line): compiler_warnings.append( { "file": match.group("file"), @@ -39,14 +40,16 @@ def extract_warnings_from_compiler_output_json( compiler_output: str, ) -> list[dict]: """ - Extracts warnings from the compiler output when using -fdiagnostics-format=json + 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. + 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: @@ -61,18 +64,24 @@ def extract_warnings_from_compiler_output_json( for warning in warning_list: locations = warning["locations"] for location in locations: + found_location = False for key in ["caret", "start", "end"]: if key in location: compiler_warnings.append( { - "file": location[key]["file"].lstrip( - "./" - ), # Remove leading current directory if present + # 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 @@ -84,19 +93,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: - file = warning["file"] - 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( 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 are associated with a file @@ -118,7 +124,7 @@ def get_unexpected_warnings( def get_unexpected_improvements( 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 @@ -183,20 +189,23 @@ 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( - f"Compiler output file does not exist: {args.compiler_output_file_path}" + f"Compiler output file does not exist:" + f" {args.compiler_output_file_path}" ) return 1 # 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 not specified. Continuing without it (no warnings ignored)." + "Warning ignore file not specified." + " Continuing without it (no warnings ignored)." ) files_with_expected_warnings = set() else: if not Path(args.warning_ignore_file_path).is_file(): print( - f"Warning ignore file does not exist: {args.warning_ignore_file_path}" + f"Warning ignore file does not exist:" + f" {args.warning_ignore_file_path}" ) return 1 with Path(args.warning_ignore_file_path).open( From f35ba6013af9db280073b0f23f5449f922f5c00a Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Mon, 5 Aug 2024 15:00:54 -0500 Subject: [PATCH 33/33] Fix comment formatting --- Tools/build/check_warnings.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index ddadde6583264b..31258932dbd4ca 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -47,7 +47,8 @@ def extract_warnings_from_compiler_output_json( 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 + # Regex to find json arrays at the top level of the file + # in the compiler output json_arrays = re.findall( r"\[(?:[^[\]]|\[[^\]]*\])*\]", compiler_output ) @@ -64,7 +65,6 @@ def extract_warnings_from_compiler_output_json( for warning in warning_list: locations = warning["locations"] for location in locations: - found_location = False for key in ["caret", "start", "end"]: if key in location: compiler_warnings.append( @@ -91,7 +91,8 @@ def extract_warnings_from_compiler_output_json( 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 + Returns a dictionary where the key is the file and the data is the warnings + in that file """ warnings_by_file = defaultdict(list) for warning in warnings: @@ -105,8 +106,9 @@ def get_unexpected_warnings( files_with_warnings: dict[str, list[dict]], ) -> int: """ - Returns failure status if warnings discovered in list of warnings are associated with a file - that is not found in the list of files with expected warnings + Returns failure status if warnings discovered in list of warnings + are associated with a file that is not found in the list of files + with expected warnings """ unexpected_warnings = [] for file in files_with_warnings.keys(): @@ -127,8 +129,8 @@ def get_unexpected_improvements( 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: @@ -171,7 +173,8 @@ def main(argv: list[str] | None = None) -> int: "--fail-on-improvement", action="store_true", default=False, - help="Flag to fail if files that were expected to have warnings have no warnings", + help="Flag to fail if files that were expected " + "to have warnings have no warnings", ) parser.add_argument( "-t",