From f7d8e99cd658f81731fdd349caf6fe98e85123cb Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 11:08:19 +0100 Subject: [PATCH 01/10] Move the C file whitespace check to pre-commit --- .pre-commit-config.yaml | 8 ++++++++ Tools/patchcheck/patchcheck.py | 28 ++++++---------------------- 2 files changed, 14 insertions(+), 22 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4c1fd20ea921b8..ad3f59fe27174d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -19,6 +19,14 @@ repos: - id: trailing-whitespace types_or: [c, python, rst] + - repo: local + hooks: + - id: c-file-whitespace + name: "Check C file whitespace" + language: pygrep + entry: '\t' + types_or: ['c', 'c++'] + - repo: https://github.com/sphinx-contrib/sphinx-lint rev: v0.6.8 hooks: diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index fa3a43af6e6048..b96fc698f7c8cb 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -191,20 +191,6 @@ def normalize_whitespace(file_paths): return fixed -@status("Fixing C file whitespace", info=report_modified_files) -def normalize_c_whitespace(file_paths): - """Report if any C files """ - fixed = [] - for path in file_paths: - abspath = os.path.join(SRCDIR, path) - with open(abspath, 'r') as f: - if '\t' not in f.read(): - continue - untabify.process(abspath, 8, verbose=False) - fixed.append(path) - return fixed - - ws_re = re.compile(br'\s+(\r?\n)$') @status("Fixing docs whitespace", info=report_modified_files) @@ -272,7 +258,6 @@ def ci(pull_request): fn.endswith(('.rst', '.inc'))] fixed = [] fixed.extend(normalize_whitespace(python_files)) - fixed.extend(normalize_c_whitespace(c_files)) fixed.extend(normalize_docs_whitespace(doc_files)) if not fixed: print('No whitespace issues found') @@ -285,14 +270,11 @@ def main(): base_branch = get_base_branch() file_paths = changed_files(base_branch) python_files = [fn for fn in file_paths if fn.endswith('.py')] - c_files = [fn for fn in file_paths if fn.endswith(('.c', '.h'))] doc_files = [fn for fn in file_paths if fn.startswith('Doc') and fn.endswith(('.rst', '.inc'))] misc_files = {p for p in file_paths if p.startswith('Misc')} # PEP 8 whitespace rules enforcement. normalize_whitespace(python_files) - # C rules enforcement. - normalize_c_whitespace(c_files) # Doc whitespace enforcement. normalize_docs_whitespace(doc_files) # Docs updated. @@ -307,10 +289,12 @@ def main(): regenerated_pyconfig_h_in(file_paths) # Test suite run and passed. - if python_files or c_files: - end = " and check for refleaks?" if c_files else "?" - print() - print("Did you run the test suite" + end) + has_c_files = any(fn for fn in file_paths if fn.endswith(('.c', '.h'))) + print() + if has_c_files: + print("Did you run the test suite and check for refleaks?") + elif python_files: + print("Did you run the test suite?") if __name__ == '__main__': From 25688d9e43f77034bce6346b58578dcea515018a Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 11:10:32 +0100 Subject: [PATCH 02/10] unused variable --- Tools/patchcheck/patchcheck.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index b96fc698f7c8cb..972aba1ad91f42 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -253,7 +253,6 @@ def ci(pull_request): base_branch = get_base_branch() file_paths = changed_files(base_branch) python_files = [fn for fn in file_paths if fn.endswith('.py')] - c_files = [fn for fn in file_paths if fn.endswith(('.c', '.h'))] doc_files = [fn for fn in file_paths if fn.startswith('Doc') and fn.endswith(('.rst', '.inc'))] fixed = [] From 6e9040c728e98853f7f79d6676e621d22d3e8f3e Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 11:31:55 +0100 Subject: [PATCH 03/10] Exclude vendored code --- .pre-commit-config.yaml | 2 ++ Tools/patchcheck/patchcheck.py | 19 ++----------------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index ad3f59fe27174d..d6addaec4bfc08 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -26,6 +26,8 @@ repos: language: pygrep entry: '\t' types_or: ['c', 'c++'] + # Don't check the style of copies directories containing external libraries + exclude: '^Modules/(_decimal/libmpdec|expat|zlib)/.*$' - repo: https://github.com/sphinx-contrib/sphinx-lint rev: v0.6.8 diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index 972aba1ad91f42..7f1fca432f2cf1 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -8,16 +8,8 @@ import sysconfig import reindent -import untabify -# Excluded directories which are copies of external libraries: -# don't check their coding style -EXCLUDE_DIRS = [ - os.path.join('Modules', '_decimal', 'libmpdec'), - os.path.join('Modules', 'expat'), - os.path.join('Modules', 'zlib'), - ] SRCDIR = sysconfig.get_config_var('srcdir') @@ -147,15 +139,8 @@ def changed_files(base_branch=None): else: sys.exit('need a git checkout to get modified files') - filenames2 = [] - for filename in filenames: - # Normalize the path to be able to match using .startswith() - filename = os.path.normpath(filename) - if any(filename.startswith(path) for path in EXCLUDE_DIRS): - # Exclude the file - continue - filenames2.append(filename) - + # Normalize the path to be able to match using .startswith() + filenames2 = list(map(os.path.normpath, filenames)) return filenames2 From 0e20e9f090ff48dd643d33a14058a69b476fac0f Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 14:32:29 +0100 Subject: [PATCH 04/10] Fix tabs when run locally --- .pre-commit-config.yaml | 4 ++-- Tools/patchcheck/untabify.py | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d6addaec4bfc08..10e3dc9b1e1fba 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -23,8 +23,8 @@ repos: hooks: - id: c-file-whitespace name: "Check C file whitespace" - language: pygrep - entry: '\t' + entry: "python Tools/patchcheck/untabify.py" + language: "system" types_or: ['c', 'c++'] # Don't check the style of copies directories containing external libraries exclude: '^Modules/(_decimal/libmpdec|expat|zlib)/.*$' diff --git a/Tools/patchcheck/untabify.py b/Tools/patchcheck/untabify.py index 861c83ced90f24..1308aeb8e55281 100755 --- a/Tools/patchcheck/untabify.py +++ b/Tools/patchcheck/untabify.py @@ -32,10 +32,10 @@ def process(filename, tabsize, verbose=True): encoding = f.encoding except IOError as msg: print("%r: I/O error: %s" % (filename, msg)) - return + return 2 newtext = text.expandtabs(tabsize) if newtext == text: - return + return 0 backup = filename + "~" try: os.unlink(backup) @@ -49,7 +49,8 @@ def process(filename, tabsize, verbose=True): f.write(newtext) if verbose: print(filename) + return 1 if __name__ == '__main__': - main() + raise SystemExit(main()) From b44f4c2774f1d40806c41b777935cf6b5b264cf3 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 16:36:00 +0100 Subject: [PATCH 05/10] Exit with the right code --- Tools/patchcheck/untabify.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Tools/patchcheck/untabify.py b/Tools/patchcheck/untabify.py index 1308aeb8e55281..5c9d1208540830 100755 --- a/Tools/patchcheck/untabify.py +++ b/Tools/patchcheck/untabify.py @@ -21,8 +21,7 @@ def main(): if optname == '-t': tabsize = int(optvalue) - for filename in args: - process(filename, tabsize) + return max(process(filename, tabsize) for filename in args) def process(filename, tabsize, verbose=True): From d4d31ff9ba73e943724bb3e78989630c7fff9a0e Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 26 Sep 2023 16:37:07 +0100 Subject: [PATCH 06/10] zlib is no more --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 10e3dc9b1e1fba..9fd41ccbbcd0f6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -27,7 +27,7 @@ repos: language: "system" types_or: ['c', 'c++'] # Don't check the style of copies directories containing external libraries - exclude: '^Modules/(_decimal/libmpdec|expat|zlib)/.*$' + exclude: '^Modules/(_decimal/libmpdec|expat)/.*$' - repo: https://github.com/sphinx-contrib/sphinx-lint rev: v0.6.8 From b70cece166d2f05c343d11d5a0d3981e4c7abaf6 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade Date: Tue, 10 Oct 2023 02:50:26 -0600 Subject: [PATCH 07/10] More concise comment --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9fd41ccbbcd0f6..be73c7f0a22a30 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -26,7 +26,7 @@ repos: entry: "python Tools/patchcheck/untabify.py" language: "system" types_or: ['c', 'c++'] - # Don't check the style of copies directories containing external libraries + # Don't check the style of vendored libraries exclude: '^Modules/(_decimal/libmpdec|expat)/.*$' - repo: https://github.com/sphinx-contrib/sphinx-lint From b15ef27d019295abb3fd560730d993bb3669efa2 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 10 Oct 2023 12:16:31 +0100 Subject: [PATCH 08/10] Remove the now-redundant --ci option --- Tools/patchcheck/patchcheck.py | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index 981b777ac57106..0fb26fb283ca7a 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -142,9 +142,8 @@ def changed_files(base_branch=None): else: sys.exit('need a git checkout to get modified files') - # Normalize the path to be able to match using .startswith() - filenames2 = list(map(os.path.normpath, filenames)) - return filenames2 + # Normalize the path to be able to match using str.startswith() + return list(map(os.path.normpath, filenames)) def report_modified_files(file_paths): @@ -201,22 +200,6 @@ def regenerated_pyconfig_h_in(file_paths): return "not needed" -def ci(pull_request): - if pull_request == 'false': - print('Not a pull request; skipping') - return - base_branch = get_base_branch() - file_paths = changed_files(base_branch) - fixed = [] - if not fixed: - print('No whitespace issues found') - else: - count = len(fixed) - print(f'Please fix the {n_files_str(count)} with whitespace issues') - print('(on Unix you can run `make patchcheck` to make the fixes)') - sys.exit(1) - - def main(): base_branch = get_base_branch() file_paths = changed_files(base_branch) @@ -245,12 +228,4 @@ def main(): if __name__ == '__main__': - import argparse - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('--ci', - help='Perform pass/fail checks') - args = parser.parse_args() - if args.ci: - ci(args.ci) - else: - main() + main() From 556978b8de51aa9a81afcab09a0717f482260ff8 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 10 Oct 2023 12:18:31 +0100 Subject: [PATCH 09/10] Remove unused imports, variables, and functions --- Tools/patchcheck/patchcheck.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index 0fb26fb283ca7a..66328c8becc1f6 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -1,8 +1,6 @@ #!/usr/bin/env python3 """Check proposed changes for common issues.""" -import re import sys -import shutil import os.path import subprocess import sysconfig @@ -146,23 +144,6 @@ def changed_files(base_branch=None): return list(map(os.path.normpath, filenames)) -def report_modified_files(file_paths): - count = len(file_paths) - if count == 0: - return n_files_str(count) - else: - lines = [f"{n_files_str(count)}:"] - for path in file_paths: - lines.append(f" {path}") - return "\n".join(lines) - - -#: Python files that have tabs by design: -_PYTHON_FILES_WITH_TABS = frozenset({ - 'Tools/c-analyzer/cpython/_parser.py', -}) - - @status("Docs modified", modal=True) def docs_modified(file_paths): """Report if any file in the Doc directory has been changed.""" From 6891bdb68638350a83c257b49098ce2cc2a67019 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Tue, 10 Oct 2023 12:24:42 +0100 Subject: [PATCH 10/10] Verbose regular expression --- .pre-commit-config.yaml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4f28d7af011aa9..56341176caeca6 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -40,7 +40,12 @@ repos: language: "system" types_or: ['c', 'c++'] # Don't check the style of vendored libraries - exclude: '^Modules/(_decimal/libmpdec|expat)/.*$' + exclude: | + (?x)^( + Modules/_decimal/.* + | Modules/libmpdec/.* + | Modules/expat/.* + )$ - repo: https://github.com/sphinx-contrib/sphinx-lint rev: v0.6.8