[clang-tidy] run-clang-tidy.py: check if clang-apply-replacements succeeds

[kuhar]

Summary

When running run-clang-tidy.py with -fix it tries to apply found replacements at the end.
If there are errors running clang-apply-replacements, the script currently crashes or displays no error at all.

This patch checks for errors running clang-apply-replacements the same way clang-tidy binary is handled.

Another option would be probably checking for clang-apply-replacements (when -fix is passed) even before running clang-tidy.

[alexfh]

clang-tidy/tool/run-clang-tidy.py

99–101 ↗

(On Diff #95952)

I'd place try/except in main around the call to apply_fixes() and also move shutil.rmtree(tmpdir) out of the apply_fixes() function.

100 ↗

(On Diff #95952)

"Unable to run clang-apply-replacements" without any details seems to be far worse than just a default stack trace from an unhandled exception. At the very least add the message from the exception.

[kimgr]

Python 3 concern inline

clang-tidy/tool/run-clang-tidy.py

100 ↗

(On Diff #95952)

I don't know what the general Python3 ambitions of this script is, but it would be nice if the new code didn't use the Python2-only print style.

You can:

from __future__ import print_function

at the top of the file, and then use Python3-style print:

print("Unable to run clang-apply-replacements", file=sys.stderr)

[kuhar]

Use python3 printing. Move exception handling out of apply_fixes.

Now, the code prints the following on failure:

Applying fixes ...
Error applying fixes. Is clang-apply-replacements binary correctly specified?

Traceback (most recent call last):
  File "/home/kuhar/projects/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py", line 217, in main
    apply_fixes(args, tmpdir)
  File "/home/kuhar/projects/llvm/tools/clang/tools/extra/clang-tidy/tool/run-clang-tidy.py", line 98, in apply_fixes
    subprocess.call(invocation)
  File "/usr/lib/python2.7/subprocess.py", line 523, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1343, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

I hope that suggesting problem with clang-apply-replacements binary and printing stack trace is more useful.

[kuhar]

clang-tidy/tool/run-clang-tidy.py

100 ↗

(On Diff #95952)

Thanks for feedback. I changed the code to print suggestion about clang-apply-replacements and stack trace. Is there a better way to make it easy to debug?

[alexfh]

SGTM provided this continues to work with python 2.

[Prazek]

LGTM, but please leave the FIXME comment about the running of apply replacement

[kuhar]

After thinking about Piotr's comment, I think that a better way to perform the check would be te invoking clang-apply-replacements with --version and seeing if it fails even before running clang-tidy.
This way it is possible to save a lot of time when run-clang-tidy.py is run on a big project and apply_fixes fails at the end.

Let me know what you think about this approach.

[Prazek]

In D32294#732861, @kuhar wrote:

After thinking about Piotr's comment, I think that a better way to perform the check would be te invoking clang-apply-replacements with --version and seeing if it fails even before running clang-tidy.
This way it is possible to save a lot of time when run-clang-tidy.py is run on a big project and apply_fixes fails at the end.

Let me know what you think about this approach.

Sounds good to me!

[alexfh]

In D32294#732986, @Prazek wrote:

In D32294#732861, @kuhar wrote:

After thinking about Piotr's comment, I think that a better way to perform the check would be te invoking clang-apply-replacements with --version and seeing if it fails even before running clang-tidy.
This way it is possible to save a lot of time when run-clang-tidy.py is run on a big project and apply_fixes fails at the end.

Let me know what you think about this approach.

Sounds good to me!

+1

[alexfh]

clang-tidy/tool/run-clang-tidy.py

226 ↗

(On Diff #96045)

= False here and = True after apply_fixes() inside try.

[kuhar]

Minor change to success detection logic.

[kuhar]

Is the current patch OK? Do you have any other comments?

[Prazek]

LGTM after the fixes

clang-tools-extra/trunk/clang-tidy/tool/run-clang-tidy.py

- Fix all header guards included from clang-tidy and header guards
   for clang-tidy headers.
     run-clang-tidy.py -fix -checks=-*,llvm-header-guard extra/clang-tidy \
                       -header-filter=extra/clang-tidy
 
 Compilation database setup:
 http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 """
 
+from __future__ import print_function
 import argparse
 import json
 import multiprocessing
 import os
 import Queue
 import re
 import shutil
 import subprocess
 import sys
 import tempfile
 import threading
+import traceback
 
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
   result = './'
   while not os.path.isfile(os.path.join(result, path)):
     if os.path.realpath(result) == '/':
-      print 'Error: could not find compilation database.'
+      print('Error: could not find compilation database.')
       sys.exit(1)
     result += '../'
   return os.path.realpath(result)
 
 
 def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
                         header_filter, extra_arg, extra_arg_before, quiet):
   """Gets a command line for clang-tidy."""
for arg in extra_arg_before:
       start.append('-extra-arg-before=%s' % arg)
   start.append('-p=' + build_path)
   if quiet:
       start.append('-quiet')
   start.append(f)
   return start
 
 
+def check_clang_apply_replacements_binary(args):
+  """Checks if invoking supplied clang-apply-replacements binary works."""
+  try:
+    subprocess.check_call([args.clang_apply_replacements_binary, '--version'])
+  except:
+    print('Unable to run clang-apply-replacements. Is clang-apply-replacements '
+          'binary correctly specified?', file=sys.stderr)
+    traceback.print_exc()
+    sys.exit(1)
+
+
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
   invocation = [args.clang_apply_replacements_binary]
   if args.format:
     invocation.append('-format')
   invocation.append(tmpdir)
   subprocess.call(invocation)
-  shutil.rmtree(tmpdir)
 
 
 def run_tidy(args, tmpdir, build_path, queue):
   """Takes filenames out of queue and runs clang-tidy on them."""
   while True:
     name = queue.get()
     invocation = get_tidy_invocation(name, args.clang_tidy_binary, args.checks,
                                      tmpdir, build_path, args.header_filter,
else:
     build_path = find_compilation_database(db_path)
 
   try:
     invocation = [args.clang_tidy_binary, '-list-checks']
     invocation.append('-p=' + build_path)
     if args.checks:
       invocation.append('-checks=' + args.checks)
     invocation.append('-')
-    print subprocess.check_output(invocation)
+    print(subprocess.check_output(invocation))
   except:
-    print >>sys.stderr, "Unable to run clang-tidy."
+    print("Unable to run clang-tidy.", file=sys.stderr)
     sys.exit(1)
 
   # Load the database and extract all files.
   database = json.load(open(os.path.join(build_path, db_path)))
   files = [entry['file'] for entry in database]
 
   max_task = args.j
   if max_task == 0:
     max_task = multiprocessing.cpu_count()
 
   tmpdir = None
   if args.fix:
+    check_clang_apply_replacements_binary(args)
     tmpdir = tempfile.mkdtemp()
 
   # Build up a big regexy filter from all command line arguments.
   file_name_re = re.compile('|'.join(args.files))
 
   try:
     # Spin up a bunch of tidy-launching threads.
     queue = Queue.Queue(max_task)
for name in files:
         queue.put(name)
 
     # Wait for all threads to be done.
     queue.join()
 
   except KeyboardInterrupt:
     # This is a sad hack. Unfortunately subprocess goes
     # bonkers with ctrl-c and we start forking merrily.
-    print '\nCtrl-C detected, goodbye.'
+    print('\nCtrl-C detected, goodbye.')
     if args.fix:
       shutil.rmtree(tmpdir)
     os.kill(0, 9)
 
   if args.fix:
-    print 'Applying fixes ...'
+    print('Applying fixes ...')
+    successfully_applied = False
+
+    try:
       apply_fixes(args, tmpdir)
+      successfully_applied = True
+    except:
+      print('Error applying fixes.\n', file=sys.stderr)
+      traceback.print_exc()
+
+    shutil.rmtree(tmpdir)
+    if not successfully_applied:
+      sys.exit(1)
 
 if __name__ == '__main__':
   main()