From 1f25035b8800bd063be6adbf598a5b38b97bd195 Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade Date: Wed, 11 Oct 2023 17:31:43 +0200 Subject: [PATCH 1/2] Revert python-file-whitespace and c-file-whitespace pending a portable solution --- .pre-commit-config.yaml | 24 ------ Tools/patchcheck/patchcheck.py | 143 ++++++++++++++++++++++++++++++--- 2 files changed, 133 insertions(+), 34 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 9a27d30fd4163e..5b362a7c9d4c8a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -23,30 +23,6 @@ repos: - id: trailing-whitespace types_or: [c, inc, python, rst] - - repo: local - hooks: - - id: python-file-whitespace - name: "Check Python file whitespace" - entry: 'python Tools/patchcheck/reindent.py --nobackup --newline LF' - language: 'system' - types: [python] - exclude: '^(Lib/test/tokenizedata/|Tools/c-analyzer/cpython/_parser).*$' - - - repo: local - hooks: - - id: c-file-whitespace - name: "Check C file whitespace" - entry: "python Tools/patchcheck/untabify.py" - language: "system" - types_or: ['c', 'c++'] - # Don't check the style of vendored libraries - exclude: | - (?x)^( - Modules/_decimal/.* - | Modules/libmpdec/.* - | Modules/expat/.* - )$ - - 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 66328c8becc1f6..b17c2b8cf856eb 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -1,15 +1,30 @@ #!/usr/bin/env python3 """Check proposed changes for common issues.""" +import re import sys +import shutil import os.path import subprocess import sysconfig +import reindent +import untabify + + def get_python_source_dir(): src_dir = sysconfig.get_config_var('abs_srcdir') if not src_dir: src_dir = sysconfig.get_config_var('srcdir') return os.path.abspath(src_dir) + + +# 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 = get_python_source_dir() @@ -140,8 +155,83 @@ 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 str.startswith() - return list(map(os.path.normpath, filenames)) + 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) + + return filenames2 + + +def report_modified_files(file_paths): + count = len(file_paths) + if count == 0: + return n_files_str(count) + else: + lines = ["{}:".format(n_files_str(count))] + for path in file_paths: + lines.append(" {}".format(path)) + return "\n".join(lines) + + +#: Python files that have tabs by design: +_PYTHON_FILES_WITH_TABS = frozenset({ + 'Tools/c-analyzer/cpython/_parser.py', +}) + + +@status("Fixing Python file whitespace", info=report_modified_files) +def normalize_whitespace(file_paths): + """Make sure that the whitespace for .py files have been normalized.""" + reindent.makebackup = False # No need to create backups. + fixed = [ + path for path in file_paths + if ( + path.endswith('.py') + and path not in _PYTHON_FILES_WITH_TABS + and reindent.check(os.path.join(SRCDIR, path)) + ) + ] + 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) +def normalize_docs_whitespace(file_paths): + fixed = [] + for path in file_paths: + abspath = os.path.join(SRCDIR, path) + try: + with open(abspath, 'rb') as f: + lines = f.readlines() + new_lines = [ws_re.sub(br'\1', line) for line in lines] + if new_lines != lines: + shutil.copyfile(abspath, abspath + '.bak') + with open(abspath, 'wb') as f: + f.writelines(new_lines) + fixed.append(path) + except Exception as err: + print('Cannot fix %s: %s' % (path, err)) + return fixed @status("Docs modified", modal=True) @@ -180,13 +270,41 @@ def regenerated_pyconfig_h_in(file_paths): else: 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) + 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 = [] + 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') + else: + print(f'Please fix the {len(fixed)} file(s) 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) + 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. docs_modified(doc_files) # Misc/ACKS changed. @@ -199,14 +317,19 @@ def main(): regenerated_pyconfig_h_in(file_paths) # Test suite run and passed. - has_c_files = any(fn for fn in file_paths if fn.endswith(('.c', '.h'))) - has_python_files = any(fn for fn in file_paths if fn.endswith('.py')) - print() - if has_c_files: - print("Did you run the test suite and check for refleaks?") - elif has_python_files: - print("Did you run the test suite?") + if python_files or c_files: + end = " and check for refleaks?" if c_files else "?" + print() + print("Did you run the test suite" + end) if __name__ == '__main__': - 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() From 105b86c819214ca2ce165f37eac55cc45ff1c4be Mon Sep 17 00:00:00 2001 From: Hugo van Kemenade Date: Wed, 11 Oct 2023 17:42:13 +0200 Subject: [PATCH 2/2] Don't revert too much --- Tools/patchcheck/patchcheck.py | 37 +++++++--------------------------- 1 file changed, 7 insertions(+), 30 deletions(-) diff --git a/Tools/patchcheck/patchcheck.py b/Tools/patchcheck/patchcheck.py index b17c2b8cf856eb..af1f0584bb5403 100755 --- a/Tools/patchcheck/patchcheck.py +++ b/Tools/patchcheck/patchcheck.py @@ -172,9 +172,9 @@ def report_modified_files(file_paths): if count == 0: return n_files_str(count) else: - lines = ["{}:".format(n_files_str(count))] + lines = [f"{n_files_str(count)}:"] for path in file_paths: - lines.append(" {}".format(path)) + lines.append(f" {path}") return "\n".join(lines) @@ -213,27 +213,6 @@ def normalize_c_whitespace(file_paths): return fixed -ws_re = re.compile(br'\s+(\r?\n)$') - -@status("Fixing docs whitespace", info=report_modified_files) -def normalize_docs_whitespace(file_paths): - fixed = [] - for path in file_paths: - abspath = os.path.join(SRCDIR, path) - try: - with open(abspath, 'rb') as f: - lines = f.readlines() - new_lines = [ws_re.sub(br'\1', line) for line in lines] - if new_lines != lines: - shutil.copyfile(abspath, abspath + '.bak') - with open(abspath, 'wb') as f: - f.writelines(new_lines) - fixed.append(path) - except Exception as err: - print('Cannot fix %s: %s' % (path, err)) - return fixed - - @status("Docs modified", modal=True) def docs_modified(file_paths): """Report if any file in the Doc directory has been changed.""" @@ -270,6 +249,7 @@ def regenerated_pyconfig_h_in(file_paths): else: return "not needed" + def ci(pull_request): if pull_request == 'false': print('Not a pull request; skipping') @@ -278,19 +258,18 @@ def ci(pull_request): 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 = [] 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') else: - print(f'Please fix the {len(fixed)} file(s) with whitespace issues') - print('(on UNIX you can run `make patchcheck` to make the fixes)') + 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) @@ -303,8 +282,6 @@ def main(): normalize_whitespace(python_files) # C rules enforcement. normalize_c_whitespace(c_files) - # Doc whitespace enforcement. - normalize_docs_whitespace(doc_files) # Docs updated. docs_modified(doc_files) # Misc/ACKS changed.