From 3749c11f21ca5648b1cc17309c2b0b6459cfd120 Mon Sep 17 00:00:00 2001 From: Oxan van Leeuwen Date: Sun, 25 Jul 2021 23:54:32 +0200 Subject: [PATCH] Fix clang-format script behaviour without -i + code cleanup (#2002) Co-authored-by: Stefan Agner --- .github/workflows/ci.yml | 9 +++-- script/clang-format | 75 ++++++++++++++++------------------------ script/clang-tidy | 46 +++++++++--------------- script/helpers.py | 4 ++- 4 files changed, 57 insertions(+), 77 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0be9be903..433c7fb872 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,16 +46,21 @@ jobs: echo "::add-matcher::.github/workflows/matchers/clang-tidy.json" echo "::add-matcher::.github/workflows/matchers/gcc.json" + # Also run git-diff-index so that the step is marked as failed on formatting errors, + # since clang-format doesn't do anything but change files if -i is passed. - name: Run clang-format - run: script/clang-format -i + run: | + script/clang-format -i + git diff-index --quiet HEAD -- if: ${{ matrix.id == 'clang-format' }} - name: Run clang-tidy run: script/clang-tidy --all-headers --fix --split-num 4 --split-at ${{ matrix.split }} if: ${{ matrix.id == 'clang-tidy' }} - - name: Suggest changes + - name: Suggested changes run: script/ci-suggest-changes + if: always() ci: # Don't use the esphome-lint docker image because it may contain outdated requirements. diff --git a/script/clang-format b/script/clang-format index bb2b722e1c..d6588f1ccb 100755 --- a/script/clang-format +++ b/script/clang-format @@ -1,10 +1,9 @@ #!/usr/bin/env python3 -from __future__ import print_function - import argparse import multiprocessing import os +import queue import re import subprocess import sys @@ -13,59 +12,47 @@ import threading import click sys.path.append(os.path.dirname(__file__)) -from helpers import basepath, get_output, git_ls_files, filter_changed - -is_py2 = sys.version[0] == '2' - -if is_py2: - import Queue as queue -else: - import queue as queue - -root_path = os.path.abspath(os.path.normpath(os.path.join(__file__, '..', '..'))) -basepath = os.path.join(root_path, 'esphome') -rel_basepath = os.path.relpath(basepath, os.getcwd()) +from helpers import get_output, git_ls_files, filter_changed -def run_format(args, queue, lock): - """Takes filenames out of queue and runs clang-tidy on them.""" +def run_format(args, queue, lock, failed_files): + """Takes filenames out of queue and runs clang-format on them.""" while True: path = queue.get() invocation = ['clang-format-11'] if args.inplace: invocation.append('-i') + else: + invocation.extend(['--dry-run', '-Werror']) invocation.append(path) - proc = subprocess.Popen(invocation, stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - output, err = proc.communicate() - with lock: - if proc.returncode != 0: - print(' '.join(invocation)) - print(output.decode('utf-8')) - print(err.decode('utf-8')) + proc = subprocess.run(invocation, capture_output=True, encoding='utf-8') + if proc.returncode != 0: + with lock: + print() + print("\033[0;32m************* File \033[1;32m{}\033[0m".format(path)) + print(proc.stdout) + print(proc.stderr) + print() + failed_files.append(path) queue.task_done() def progress_bar_show(value): - if value is None: - return '' - return value + return value if value is not None else '' def main(): parser = argparse.ArgumentParser() parser.add_argument('-j', '--jobs', type=int, default=multiprocessing.cpu_count(), - help='number of tidy instances to be run in parallel.') + help='number of format instances to be run in parallel.') parser.add_argument('files', nargs='*', default=[], help='files to be processed (regex on path)') parser.add_argument('-i', '--inplace', action='store_true', - help='apply fix-its') - parser.add_argument('-q', '--quiet', action='store_false', - help='Run clang-tidy in quiet mode') + help='reformat files in-place') parser.add_argument('-c', '--changed', action='store_true', - help='Only run on changed files') + help='only run on changed files') args = parser.parse_args() try: @@ -75,7 +62,7 @@ def main(): Oops. It looks like clang-format is not installed. Please check you can run "clang-format-11 -version" in your terminal and install - clang-format (v7) if necessary. + clang-format (v11) if necessary. Note you can also upload your code as a pull request on GitHub and see the CI check output to apply clang-format. @@ -83,28 +70,26 @@ def main(): return 1 files = [] - for path in git_ls_files(): - filetypes = ('.cpp', '.h', '.tcc') - ext = os.path.splitext(path)[1] - if ext in filetypes: - path = os.path.relpath(path, os.getcwd()) - files.append(path) - # Match against re - file_name_re = re.compile('|'.join(args.files)) - files = [p for p in files if file_name_re.search(p)] + for path in git_ls_files(['*.cpp', '*.h', '*.tcc']): + files.append(os.path.relpath(path, os.getcwd())) + + if args.files: + # Match against files specified on command-line + file_name_re = re.compile('|'.join(args.files)) + files = [p for p in files if file_name_re.search(p)] if args.changed: files = filter_changed(files) files.sort() - return_code = 0 + failed_files = [] try: task_queue = queue.Queue(args.jobs) lock = threading.Lock() for _ in range(args.jobs): t = threading.Thread(target=run_format, - args=(args, task_queue, lock)) + args=(args, task_queue, lock, failed_files)) t.daemon = True t.start() @@ -122,7 +107,7 @@ def main(): print('Ctrl-C detected, goodbye.') os.kill(0, 9) - sys.exit(return_code) + sys.exit(len(failed_files)) if __name__ == '__main__': diff --git a/script/clang-tidy b/script/clang-tidy index 0bf17f9076..11b9818016 100755 --- a/script/clang-tidy +++ b/script/clang-tidy @@ -1,10 +1,9 @@ #!/usr/bin/env python3 -from __future__ import print_function - import argparse import multiprocessing import os +import queue import re import shutil import subprocess @@ -19,13 +18,6 @@ sys.path.append(os.path.dirname(__file__)) from helpers import basepath, shlex_quote, get_output, build_compile_commands, \ build_all_include, temp_header_file, git_ls_files, filter_changed -is_py2 = sys.version[0] == '2' - -if is_py2: - import Queue as queue -else: - import queue as queue - def run_tidy(args, tmpdir, queue, lock, failed_files): while True: @@ -49,8 +41,8 @@ def run_tidy(args, tmpdir, queue, lock, failed_files): # Use pexpect for a pseudy-TTY with colored output output, rc = pexpect.run(invocation_s, withexitstatus=True, encoding='utf-8', timeout=15 * 60) - with lock: - if rc != 0: + if rc != 0: + with lock: print() print("\033[0;32m************* File \033[1;32m{}\033[0m".format(path)) print(output) @@ -78,15 +70,15 @@ def main(): help='files to be processed (regex on path)') parser.add_argument('--fix', action='store_true', help='apply fix-its') parser.add_argument('-q', '--quiet', action='store_false', - help='Run clang-tidy in quiet mode') + help='run clang-tidy in quiet mode') parser.add_argument('-c', '--changed', action='store_true', - help='Only run on changed files') - parser.add_argument('--split-num', type=int, help='Split the files into X jobs.', + help='only run on changed files') + parser.add_argument('--split-num', type=int, help='split the files into X jobs.', default=None) - parser.add_argument('--split-at', type=int, help='Which split is this? Starts at 1', + parser.add_argument('--split-at', type=int, help='which split is this? starts at 1', default=None) parser.add_argument('--all-headers', action='store_true', - help='Create a dummy file that checks all headers') + help='create a dummy file that checks all headers') args = parser.parse_args() try: @@ -107,15 +99,13 @@ def main(): build_compile_commands() files = [] - for path in git_ls_files(): - filetypes = ('.cpp',) - ext = os.path.splitext(path)[1] - if ext in filetypes: - path = os.path.relpath(path, os.getcwd()) - files.append(path) - # Match against re - file_name_re = re.compile('|'.join(args.files)) - files = [p for p in files if file_name_re.search(p)] + for path in git_ls_files(['*.cpp']): + files.append(os.path.relpath(path, os.getcwd())) + + if args.files: + # Match against files specified on command-line + file_name_re = re.compile('|'.join(args.files)) + files = [p for p in files if file_name_re.search(p)] if args.changed: files = filter_changed(files) @@ -133,7 +123,6 @@ def main(): tmpdir = tempfile.mkdtemp() failed_files = [] - return_code = 0 try: task_queue = queue.Queue(args.jobs) lock = threading.Lock() @@ -151,7 +140,6 @@ def main(): # Wait for all threads to be done. task_queue.join() - return_code = len(failed_files) except KeyboardInterrupt: print() @@ -168,8 +156,8 @@ def main(): print('Error applying fixes.\n', file=sys.stderr) raise - return return_code + sys.exit(len(failed_files)) if __name__ == '__main__': - sys.exit(main()) + main() diff --git a/script/helpers.py b/script/helpers.py index 1a4402aa1d..e0ec2d169e 100644 --- a/script/helpers.py +++ b/script/helpers.py @@ -145,8 +145,10 @@ def filter_changed(files): return files -def git_ls_files(): +def git_ls_files(patterns=None): command = ["git", "ls-files", "-s"] + if patterns is not None: + command.extend(patterns) proc = subprocess.Popen(command, stdout=subprocess.PIPE) output, err = proc.communicate() lines = [x.split() for x in output.decode("utf-8").splitlines()]