From 5148727e060c947a3fc0ebb942e4ce2dc2dad2e7 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Sat, 13 Jul 2024 12:50:17 -0500 Subject: [PATCH 01/19] 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/19] 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/19] 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/19] 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/19] 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/19] 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/19] 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/19] 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/19] 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/19] =?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/19] 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/19] 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/19] 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/19] 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 c19d953df7a52ebcedf453aa63e2af513a0d08e5 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Wed, 24 Jul 2024 20:26:11 -0500 Subject: [PATCH 15/19] Add fdiagnostics to configure for cache consistency --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index fc5b98f0220626..651bc277fd930d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -307,7 +307,7 @@ jobs: with: save: false - name: Configure CPython - run: ./configure --config-cache --enable-slower-safety --with-pydebug --with-openssl=$OPENSSL_DIR + run: ./configure CFLAGS="-fdiagnostics-format=json" --config-cache --enable-slower-safety --with-pydebug --with-openssl=$OPENSSL_DIR - name: Build CPython run: make -j4 - name: Display build info From b9b95005a567ba7e22696ca4f99da47c0197b6e7 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Wed, 24 Jul 2024 20:57:45 -0500 Subject: [PATCH 16/19] 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 1330a14f91ba5d..a439e6f99f8cd4 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -31,7 +31,6 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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 0c0fc31105963a855053eea040ca05b44a6d050f Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Fri, 26 Jul 2024 17:40:02 -0500 Subject: [PATCH 17/19] Enforce PEP8 on check_warnigns --- Tools/build/check_warnings.py | 67 +++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index a439e6f99f8cd4..b7a37b8985d8cd 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -9,6 +9,7 @@ import sys from pathlib import Path + def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: """ Extracts warnings from the compiler output when using -fdiagnostics-format=json @@ -18,37 +19,48 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: """ # Regex to find json arrays at the top level of the file in the compiler output - json_arrays = re.findall(r'\[(?:[^\[\]]|\[(?:[^\[\]]|\[[^\[\]]*\])*\])*\]', 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']) + 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 + 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'] + locations = warning["locations"] for location in locations: - for key in ['caret', 'start', 'end']: + for key in ["caret", "start", "end"]: if key in location: - file = location[key]['file'] - file = file.lstrip('./') # Remove leading current directory if present + 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], @@ -71,6 +83,7 @@ def get_unexpected_warnings( return 0 + def get_unexpected_improvements( warnings: list[dict], files_with_expected_warnings: set[str], @@ -93,31 +106,32 @@ def get_unexpected_improvements( 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 compiler output file" + help="Path to the compiler output file", ) parser.add_argument( "--warning-ignore-file-path", type=str, required=True, - help="Path to the warning ignore file" + 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" + 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" + help="Flag to fail if files that were expected to have warnings have no warnings", ) args = parser.parse_args(argv) @@ -126,34 +140,49 @@ 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}") + 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}") + 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: + 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) + 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) - if args.fail_on_regression: exit_code |= status + 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, files_with_warnings) - if args.fail_on_improvement: exit_code |= status + status = get_unexpected_improvements( + warnings, files_with_expected_warnings, files_with_warnings + ) + if args.fail_on_improvement: + exit_code |= status return exit_code + if __name__ == "__main__": sys.exit(main()) From e568d6c1df5c36b3d865c5836e2358ef94349546 Mon Sep 17 00:00:00 2001 From: Nate Ohlson Date: Fri, 26 Jul 2024 17:41:21 -0500 Subject: [PATCH 18/19] Add period 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 4e8a8b6dd36ba6..d5718ed4be7606 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 +Patch by Nate Ohlson. From 9f6281efc164df8e9338f0aa1fa80349350a4290 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Date: Sat, 27 Jul 2024 12:33:17 +0300 Subject: [PATCH 19/19] Wrap comments and strings to 79 chars --- Tools/build/check_warnings.py | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/Tools/build/check_warnings.py b/Tools/build/check_warnings.py index b7a37b8985d8cd..f0c0067f4ab255 100644 --- a/Tools/build/check_warnings.py +++ b/Tools/build/check_warnings.py @@ -1,7 +1,7 @@ #!/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. +Parses compiler output with -fdiagnostics-format=json and checks that warnings +exist only in files that are expected to have warnings. """ import argparse import json @@ -12,13 +12,15 @@ def extract_warnings_from_compiler_output(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 + # Regex to find json arrays at the top level of the file + # in the compiler output json_arrays = re.findall( r"\[(?:[^\[\]]|\[(?:[^\[\]]|\[[^\[\]]*\])*\])*\]", compiler_output ) @@ -42,7 +44,8 @@ def extract_warnings_from_compiler_output(compiler_output: str) -> list[dict]: 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 = {} for warning in warnings: @@ -67,8 +70,9 @@ def get_unexpected_warnings( 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 + 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(): @@ -90,8 +94,8 @@ def get_unexpected_improvements( 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 + 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: @@ -131,7 +135,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", ) args = parser.parse_args(argv) @@ -141,14 +146,16 @@ 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}" + "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(): print( - f"Warning ignore file does not exist: {args.warning_ignore_file_path}" + "Warning ignore file does not exist: " + f"{args.warning_ignore_file_path}" ) return 1