[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

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

"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

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

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

219

= 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-tidy/tool/run-clang-tidy.py

def get_tidy_invocation(f, clang_tidy_binary, checks, tmpdir, build_path,
   if quiet:
       start.append('-quiet')
   start.append(f)
   return start
 
 
 def apply_fixes(args, tmpdir):
   """Calls clang-apply-fixes on a given directory. Deletes the dir when done."""
+  try:
     invocation = [args.clang_apply_replacements_binary]
     if args.format:
       invocation.append('-format')
     invocation.append(tmpdir)
     subprocess.call(invocation)
     shutil.rmtree(tmpdir)
+  except:
+    print >>sys.stderr, "Unable to run clang-apply-replacements."
+    sys.exit(1)
 
 
 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,
except KeyboardInterrupt:
     print '\nCtrl-C detected, goodbye.'
     if args.fix:
       shutil.rmtree(tmpdir)
     os.kill(0, 9)
 
   if args.fix:
     print 'Applying fixes ...'
     apply_fixes(args, tmpdir)
 
 if __name__ == '__main__':
   main()