This is an archive of the discontinued LLVM Phabricator instance.

Setup clang-format as an Arcanist linter
ClosedPublic

Authored by starsid on Jul 9 2018, 6:17 PM.

Details

Summary

This uses clang-format-diff as a linter for Arcanist.

arc lint flow, also run as part of arc diff unless skipped with
--nolint, will now run the linter shell script on the changed files,
and prompt the user to accept the suggested changes.

Message when clang-format-diff is not installed:

Example of the noise during code review when clang-format-diff is not installed:
https://reviews.llvm.org/differential/changeset/?ref=1115809

Prompt when clang-format-diff is installed and suggests edits:

Diff Detail

Event Timeline

starsid created this revision.Jul 9 2018, 6:17 PM
starsid updated this revision to Diff 154747.Jul 9 2018, 6:24 PM

typo in comments

dberris added inline comments.
.arcanist/linters/clang-format.sh
12–13 ↗(On Diff #154747)

What happens when the script exits with a non-zero value? Does it stop the upload/update of the patch? I suspect we don't really want to make that a hard requirement yet, unless we get agreement from llvm-dev that this is something we should enforce for all patches moving forward.

.arclint
9

Should this also include '.cc' files?

I'd move the script into utils/arcanist instead of creating a (although hidden) top level directory.

starsid updated this revision to Diff 154848.Jul 10 2018, 11:11 AM

Move linter script to utils/arcanist.
Add message on how to skip linting.

starsid updated this revision to Diff 154849.Jul 10 2018, 11:11 AM

Also include .cc files

starsid marked 2 inline comments as done.Jul 10 2018, 11:20 AM
starsid added inline comments.
.arcanist/linters/clang-format.sh
12–13 ↗(On Diff #154747)

When the script exits with a non-zero value (which should only happen if clang-format-diff is not installed), the user's upload workflow is stopped, and the user is messaged saying that the linter failed.

I have now added a message saying that the user can skip linting by passing --nolint to arc diff. So it's not a hard requirement, but it is a little disruptive. I can make it silently fail by returning an exit code of 0 instead of 1, but I don't feel like it's a good idea.

starsid edited the summary of this revision. (Show Details)Jul 10 2018, 11:26 AM
dberris added inline comments.Jul 10 2018, 4:14 PM
.arcanist/linters/clang-format.sh
12–13 ↗(On Diff #154747)

I would push back on this a bit -- let's make it noisy when you don't have clang-format installed, but not stop the upload flow and provide a way to silence the warning. When we (llvm-dev) decide that we will require clang-format'ing all new patches through arcanist/Phabricator then we can change the default.

.arclint
9

I don't see the change to support .cc files...

starsid edited the summary of this revision. (Show Details)Jul 10 2018, 11:51 PM
starsid updated this revision to Diff 154938.Jul 10 2018, 11:52 PM
starsid marked an inline comment as done.

Make the lint flow completely non-disruptive when clang-format-diff is not installed.

starsid marked an inline comment as done.Jul 10 2018, 11:56 PM

PTAL. I have updated the revision summary to demonstrate how the flows will look like.

.arclint
9

I think this regex captures all the files we want to see -- "(\\.(cc|cpp|h)$)" -- no?

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

.arclint
9

Ah, yes. I was looking at an old version of the diff.

starsid marked an inline comment as done.Jul 11 2018, 12:23 AM

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

I don't have a Windows machine to test, but maybe someone can port the bash script to python, while testing it on Windows. I don't mind if we don't commit this revision now, but someone takes over, and changes it to work on Windows.

Would it make clang-format linker available to Phabricator automatically or it would still be required to be run manually by those submitting the diffs?

Would it make clang-format linker available to Phabricator automatically or it would still be required to be run manually by those submitting the diffs?

People will need to install it once manually but it will run automatically on every arc diff.

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

The revision is up for grabs. Last I checked, the only issue was that I did not have a Windows machine to test the change.

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

The revision is up for grabs. Last I checked, the only issue was that I did not have a Windows machine to test the change.

@MyDeveloperDay your on windows right? If you have a moment, could you check that out? :)

I run on Windows 10 with cygwin

I made a whitespace change using notepad++ in llvm/lib/CommandLine.cpp, then ran

arc lint

I get the following

$ arc lint
>>> Lint for lib/Support/CommandLine.cpp:


   Auto-Fix  (S&RX) Lint
    clang-format suggested style edits found:

             273
             274   void printOptionValues();
             275
    >>> -    276   void
        -    277     registerCategory(OptionCategory *cat) {
        +          void registerCategory(OptionCategory *cat) {
             278     assert(count_if(RegisteredOptionCategories,
             279                     [cat](const OptionCategory *Category) {
             280              return cat->getName() == Category->getName();
             281            }) == 0 &&
--- /buildareas/clang/llvm/lib/Support/CommandLine.cpp  2019-01-25 14:07:42.608422800 +0000
+++ /tmp/659mblmvmukogwc4/21064-Sz7Mx7  2019-01-25 14:08:06.033582000 +0000
@@ -273,8 +273,7 @@

   void printOptionValues();

-  void
-    registerCategory(OptionCategory *cat) {
+  void registerCategory(OptionCategory *cat) {
     assert(count_if(RegisteredOptionCategories,
                     [cat](const OptionCategory *Category) {
              return cat->getName() == Category->getName();


    Apply this patch to lib/Support/CommandLine.cpp? [Y/n] n

 OKAY  No lint warnings.

If I try and run this in cmd.exe I get

$ cmd
Microsoft Windows [Version 10.0.17134.523]
(c) 2018 Microsoft Corporation. All rights reserved.

C:\cygwin64\Repos\clang\llvm\lib\Support>arc lint
PHP Warning:  require_once(/Repos/clang/llvm/lib/Support/__init_script__.php): failed to open stream: No such file or directory in /Repos/clang/llvm/lib/Support/C:\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6
PHP Fatal error:  require_once(): Failed opening required '/Repos/clang/llvm/lib/Support/__init_script__.php' (include_path='.:/usr/share/pear:/usr/share/php/php') in /Repos/clang/llvm/lib/Support/C:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6

C:\Repos\clang\llvm\lib\Support>

but this is more likely because arc help doesn't even work inside the cmd prompt... (I don't use cmd.exe)

c:\cygwin64\Repos\clang\llvm\lib\Support>arc help
PHP Warning:  require_once(/Repos/clang/llvm/lib/Support/__init_script__.php): failed to open stream: No such file or directory in /Repos/clang/llvm/lib/Support/c:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6
PHP Fatal error:  require_once(): Failed opening required '/Repos/clang/llvm/lib/Support/__init_script__.php' (include_path='.:/usr/share/pear:/usr/share/php/php') in /Repos/clang/llvm/lib/Support/c:\cygwin64\Repos\arcanist\arcanist\bin\..\scripts\arcanist.php on line 6

If its important that it works in cmd.exe (why would anyone develop inside cmd.exe!!) then I can try and get it working..

Slight aside there is already a large amount of things in LLVM that don't work in the cmd.exe prompt (or even sometimes in cygwin), so I personally wouldn't set that as a gate for this.

However once you add a .arclint file in the root then I believe all arc diff will run the lint UNLESS you do arc diff --no-lint, but that's probably been mentioned before.

That could easily catch people out.. but there are workarounds using arc alias (https://secure.phabricator.com/Q113)

@JonasToth we could have a got at making an .arclint configuration that would run the validate_check.py with the --rst option to pass a rst filename

@JonasToth we could have a got at making an .arclint configuration that would run the validate_check.py with the --rst option to pass a rst filename

Didn't get that one :D But yes, adding the validate_check.py (and maybe even more) to .arclint would be great. But lets wait first if we get fallout from this patch (like people complaining) and then continue :)

Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 2:46 AM

+1 from me too. It is a little annoying that diffs can become less clear when old code is touched and gets reformatted along with a real change, but I think the alternative of (nearly) ever commit being preceded by an NFC change to fix surrounding formatting is even worse.

Based on the comments, I don't think the revision needs more work. It is just waiting for an accept.

scott.linder accepted this revision.EditedMar 26 2020, 1:44 PM
scott.linder added a reviewer: scott.linder.

I don't know if I'm the right person to approve this, but it seems like there is pretty reasonable consensus that this is a good thing, and with Harbormaster beginning to mark formatting errors with big scary red X's I think landing this sooner rather than later will cut down on some of the review noise.

I left a small nit, but I think the script looks good as-is.

utils/arcanist/clang-format.sh
14

This could be changed to use hash, which is a bit shorter and means you skip the PATH lookup later.

i.e. if ! hash clang-format-diff 2>/dev/null

This revision is now accepted and ready to land.Mar 26 2020, 1:44 PM
starsid marked an inline comment as done.Mar 26 2020, 3:31 PM

Thank you. I made the change as per your nit. Now landing the change.

starsid updated this revision to Diff 252998.Mar 26 2020, 3:32 PM

use hash instead of command -v

Thank you. I made the change as per your nit. Now landing the change.

Actually, I can't land. Someone with write permissions will have to land.

I'll land this today.

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.
utils/arcanist/clang-format.sh
14

I have clang-format installed, I can run git clang-format as well, but I don't have a clang-format-diff executable and I don't even know how to get it.

However I see clang/tools/clang-format/clang-format-diff.py in the repo, can we call this directly?

mehdi_amini added inline comments.Apr 2 2020, 3:36 PM
utils/arcanist/clang-format.sh
44

Can we just use git clang-format indeed?

47

This does not seem relevant now that we're on GitHub.

starsid marked 3 inline comments as done.Apr 2 2020, 4:18 PM
starsid added inline comments.
utils/arcanist/clang-format.sh
14

Sure, clang-format-diff was a suggestion from the last code review. I don't think git-clang-format was available at the time. Please feel free to send an update to people to get it approved and landed.

44

I think so. Please send a review.

47

Can be replaced with an error message I believe.

mehdi_amini added inline comments.Apr 2 2020, 4:48 PM
utils/arcanist/clang-format.sh
14

Do you mind addressing all these now? You just landed it I'd expect you to be in position to do this.

starsid marked 2 inline comments as done.Apr 2 2020, 9:06 PM
starsid added inline comments.
utils/arcanist/clang-format.sh
14

One concern here would be whether this would even work on Windows, and whether it might make more sense to use a more portable scripting language to do it instead (say, Python). I don't use Windows so I'll let someone else who does (and uses Arcanist as well) to chime in as to whether this is going to be a problem.

I don't have a Windows machine to test, but maybe someone can port the bash script to python, while testing it on Windows. I don't mind if we don't commit this revision now, but someone takes over, and changes it to work on Windows.

I use arcanist on Windows via msys2, and out of the box, this does indeed fail to launch clang-format.sh (giving the normal error about Windows not knowing how to execute that kind of file -- sorry, the specific shell history is gone now). I've been passing --nolint and typing in a warning every time, which is a bit annoying. I suspect that since arcanist is a native windows command, it is trying to launch the script "natively", whereas if arcanist is installed through some other means (cygwin? msys2 package?), this might have a better time.

The next time I do some arcanist patches work on Windows, I'll see if I can fiddle with it. But in general, +1 on python vs bash as likely being more portable.

rnk added a subscriber: rnk.Apr 24 2020, 5:50 PM

This doesn't work on Windows:

$ arc diff
Linting...
 Exception
Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      utils/arcanist/clang-format.sh "lld/COFF/Driver.cpp"

      STDOUT
      (empty)

      STDERR
      'utils' is not recognized as an internal or external command,
      operable program or batch file.

(Run with `--trace` for a full exception trace.)

I would complain that one should not assume bash exists when writing portable code, but now that LLVM has git as a dependency, we can assume bash exists. I have a fix I can send along.