This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kuhar on Apr 20 2017, 7:17 AM.

Details

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.

Diff Detail

Event Timeline

kuhar created this revision.Apr 20 2017, 7:17 AM
alexfh requested changes to this revision.Apr 20 2017, 8:59 AM
alexfh added inline comments.
clang-tidy/tool/run-clang-tidy.py
116–118

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.

117

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

This revision now requires changes to proceed.Apr 20 2017, 8:59 AM
kimgr added a subscriber: kimgr.Apr 20 2017, 9:05 AM

Python 3 concern inline

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

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 updated this revision to Diff 96001.Apr 20 2017, 12:32 PM
kuhar edited edge metadata.

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 marked 2 inline comments as done.Apr 20 2017, 12:34 PM
kuhar added inline comments.
clang-tidy/tool/run-clang-tidy.py
117

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 accepted this revision.Apr 20 2017, 12:43 PM

SGTM provided this continues to work with python 2.

This revision is now accepted and ready to land.Apr 20 2017, 12:43 PM
Prazek accepted this revision.Apr 20 2017, 3:15 PM

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

kuhar updated this revision to Diff 96045.Apr 20 2017, 4:15 PM

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.

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!

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 added inline comments.Apr 20 2017, 5:24 PM
clang-tidy/tool/run-clang-tidy.py
227

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

kuhar updated this revision to Diff 96097.Apr 20 2017, 10:49 PM

Minor change to success detection logic.

kuhar marked an inline comment as done.Apr 20 2017, 10:49 PM
kuhar requested review of this revision.Apr 24 2017, 6:24 AM
kuhar edited edge metadata.

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

Prazek accepted this revision.Apr 24 2017, 6:33 AM

LGTM after the fixes

This revision is now accepted and ready to land.Apr 24 2017, 6:33 AM
This revision was automatically updated to reflect the committed changes.