Fix clang-format script behaviour without -i + code cleanup (#2002)

Co-authored-by: Stefan Agner <stefan@agner.ch>

.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.

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
+from helpers import 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())
 
 
-def run_format(args, queue, lock):
+def run_format(args, queue, lock, failed_files):
-    """Takes filenames out of queue and runs clang-tidy on them."""
+    """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,
+        proc = subprocess.run(invocation, capture_output=True, encoding='utf-8')
-                                stderr=subprocess.PIPE)
-        output, err = proc.communicate()
-        with lock:
         if proc.returncode != 0:
-                print(' '.join(invocation))
+            with lock:
-                print(output.decode('utf-8'))
+                print()
-                print(err.decode('utf-8'))
+                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 value if value is not None else ''
-        return ''
-    return value
 
 
 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')
+                        help='reformat files in-place')
-    parser.add_argument('-q', '--quiet', action='store_false',
-                        help='Run clang-tidy in quiet mode')
     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,13 +70,11 @@ def main():
         return 1
 
     files = []
-    for path in git_ls_files():
+    for path in git_ls_files(['*.cpp', '*.h', '*.tcc']):
-        filetypes = ('.cpp', '.h', '.tcc')
+        files.append(os.path.relpath(path, os.getcwd()))
-        ext = os.path.splitext(path)[1]
+
-        if ext in filetypes:
+    if args.files:
-            path = os.path.relpath(path, os.getcwd())
+        # Match against files specified on command-line
-            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)]
 
@@ -98,13 +83,13 @@ def main():
 
     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__':

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:
+            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')
+                        help='only run on changed files')
-    parser.add_argument('--split-num', type=int, help='Split the files into X jobs.',
+    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,13 +99,11 @@ def main():
     build_compile_commands()
 
     files = []
-    for path in git_ls_files():
+    for path in git_ls_files(['*.cpp']):
-        filetypes = ('.cpp',)
+        files.append(os.path.relpath(path, os.getcwd()))
-        ext = os.path.splitext(path)[1]
+
-        if ext in filetypes:
+    if args.files:
-            path = os.path.relpath(path, os.getcwd())
+        # Match against files specified on command-line
-            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)]
 
@@ -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()

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()]