952379 - Add clang-format command

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Add a helper command to mach that runs clang-format-diff.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I apologise in advance for my total lack of Python skill.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8350456 [details] [diff] [review]
Add a clang-format-diff helper to mach;

Review of attachment 8350456 [details] [diff] [review]:
-----------------------------------------------------------------

I love what you are trying to do!

Since you need to create a new patch anyway, let's take a step back for a moment.

For just a little bit more effort, I think we could provide something much better: in-place updating.

Here's what I propose:

1) Add http://selenic.com/repo/python-hglib to the tree (or we could install it via pip in the mach command until its in-tree use is justified)
2) Open a pipe to the Mercurial command server using that client library
3) Programmatically read the affected lines of a diff from revisions specified on the command line to this command. Default tip^
4) Build a clang-format process invocation with -lines arguments specifying the lines that changed
5) In-place update the source file(s) (optionally)

Perhaps that mode is engaged by passing -i to the mach command and only works if .hg is present (we need to consider the Git crowd, after all - but we could install Dulwich (a Python implementation of Git) and read diff data that way as well).

What do you think? I'm just trying to consider how valuable formatting diffs are. Chances you just want to reapply them anyway. Your source dir is under version control: you can always undo the formatting via |hg revert -C --all| (or its Git equivalent).

::: tools/mach_commands.py
@@ +320,5 @@
> +        os.chdir(self.topsrcdir)
> +
> +        from subprocess import Popen, PIPE
> +        p1 = Popen(["hg", "diff", "-U0", "-r", "tip^"], stdout=PIPE)
> +        p2 = Popen(["clang-format-diff-3.4", "-p1", "-style=Mozilla"], stdin=p1.stdout, stdout=PIPE)

This will only work if Clang 3.4 is installed. That seems fragile.

We can use the which package to find appropriate binaries on $PATH. It will automagically find .exe files on Windows!

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Gregory Szorc [:gps] from comment #4)
> For just a little bit more effort, I think we could provide something much
> better: in-place updating.

clang-format-diff is a simple python script that updates the files in place, pretty much in the way you describe.

> Here's what I propose:
> 
> 1) Add http://selenic.com/repo/python-hglib to the tree (or we could install
> it via pip in the mach command until its in-tree use is justified)
> 2) Open a pipe to the Mercurial command server using that client library
What value does this add?

> 3) Programmatically read the affected lines of a diff from revisions
> specified on the command line to this command. Default tip^
> 4) Build a clang-format process invocation with -lines arguments specifying
> the lines that changed
This is basically what clang-format-diff does.

> 5) In-place update the source file(s) (optionally)
At the moment this isn't optional. It just updates them in place. It could be out-of-place for bzexport.

> Perhaps that mode is engaged by passing -i to the mach command and only
> works if .hg is present (we need to consider the Git crowd, after all - but
> we could install Dulwich (a Python implementation of Git) and read diff data
> that way as well).

It doesn't seem unreasonable to me to just launch git or hg because it is simple and works. It is something you can always change after you find people are using the command. For git you just use:

    git diff -U0 HEAD^

We could just put this in place of the .hg command in the presence of a git repo.

> What do you think? I'm just trying to consider how valuable formatting diffs
> are. Chances you just want to reapply them anyway. Your source dir is under
> version control: you can always undo the formatting via |hg revert -C --all|
> (or its Git equivalent).

This command reformats in place, which isn't obvious from the clang-format-diff name. The workflow is something like this:

$ hg qrefresh
$ mach clang-format
$ hg diff
...look at changes...
$ hg qrefresh

At the end you can go "hg revert --all" if you don't like the formatting. Over time people will hopefully gain trust.

> This will only work if Clang 3.4 is installed. That seems fragile.

We will get most consistent results by locking down the version and providing a web service for people who don't have the correct version installed. I don't know if there are any significant differences between 3.3 and 3.4 or whether it would be helpful to fall back.

Comment 6

[Gregory Szorc [:gps]]

Ah, I didn't realize clang-format-diff inline replaced the source files. I figured it emitted a revised diff which you could then pipe into patch to apply.

Comment 7

[(no longer active)]

Hold on, clang-format doesn't yet interact well with all parts of our code.  I think adding support for it to mach is very premature at this point as it can generate bad results.

Comment 8

[Gregory Szorc [:gps]]

Perfect is the enemy of good? I think people are intelligent enough to look at the results and reject them if they look bad. If nothing else, things will get caught by code review, right?

Comment 9

[(no longer active)]

(In reply to comment #8)
> Perfect is the enemy of good?

No, broken is the enemy of the good!

> I think people are intelligent enough to look at
> the results and reject them if they look bad. If nothing else, things will get
> caught by code review, right?

I don't know how to respond to that.  Last time I tried clang-format on some parts of our code the results were terrible because we have files which don't really have a common style and that throws off clang-format making it make the wrong choices.  The result is something that is strictly worse than what you originally had.  I have actually mostly gave up using clang-format for now for that reason, it was just too unreliable.  But if you want to proceed here, then I won't hold things off.

Comment 10

[Gregory Szorc [:gps]]

I'm not the one asking for this. I'd like something in the tree to easily perform sane code formatting. If clang-format can't do that for C++, I don't know what can.

Comment 11

[(no longer active)]

I should also say that I last tried clang-format a while ago, it's possible that it has gotten better since.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8350460 [details] [diff] [review]
Add a clang-format-diff helper to mach;

Review of attachment 8350460 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/mach_commands.py
@@ +321,5 @@
> +
> +        from subprocess import Popen, PIPE
> +        p1 = Popen(["hg", "diff", "-U0", "-r", "tip^"], stdout=PIPE)
> +        p2 = Popen(["clang-format-diff-3.4", "-p1", "-style="{BasedOnStyle: Mozilla, BreakBeforeBraces: Linux}"], stdin=p1.stdout, stdout=PIPE)
> +        p2.communicate()[0]

We should search for clang-format-diff binaries more intelligently.

We should proxy the exit code of clang-format-diff via return.

What's the output of clang-format-diff? Should we send it to the terminal's stdout?

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Modified clang-format build for Linux64

clang-format-3.5 is much closer to official style. I have modified the defaults for the Mozilla style and made it put a line break between method type and method name when the methods are at the top level.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Modifications for clang-format-3.5

Comment 16

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8356925 [details] [diff] [review]
Add a clang-format-diff helper to mach;

My latest mach patch downloads the correct version but I've only built it for Linux64 so far.

Comment 17

[(no longer active)]

Anthony, can you please upstream your changes to clang?  (I'm curious to see the diff if possible!)

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The diff is attached to this bug. The changes aren't ready to be upstreamed yet. I figure we get a few people to try it out first and then follow up with upstreaming it.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8357339 [details] [diff] [review]
Add a clang-format-diff helper to mach;

I'm wondering whether the command should be "mach format" in anticipation of adding other formatting mechanisms for other languages.

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8357339 [details] [diff] [review]
Add a clang-format-diff helper to mach;

Review of attachment 8357339 [details] [diff] [review]:
-----------------------------------------------------------------

I'll be honest, I'm not a huge fan of downloading and running binaries. IMO it's one thing if we're downloading source and even compiling that source. But there's just too much that can go wrong with pre-compiled binaries (32 vs 64 bit, shared library dependencies, etc). And, I think some may even have security concerns with the current approach. I, for one, trust you. But others are more paranoid. Perhaps adding a comment and possibly a "press any key to continue" before downloading will appease this group?

I think a better approach would be to add installing an appropriate clang-format to the bootstrapper in python/mozboot/. At least there we already have the separate code paths for the different platforms and OS's more clearly defined. But that tool notably doesn't yet support Windows. So maybe the current approach is fine.

I think I'm fine with doing people downloads as a stop-gap. But I'd really like to know what the path to a more robust solution is before granting r+. If you feel this is unreasonable, please push back.

::: tools/mach_commands.py
@@ +342,5 @@
> +        p2 = Popen([sys.executable, clang_format_diff, "-p1", "-style=Mozilla"], stdin=p1.stdout)
> +        return p2.communicate()[0]
> +
> +    def locate_or_fetch(self, root):
> +        target = os.path.expanduser('~/.mozbuild/' + root)

self.state_path IIRC stores the currently configured state path (which may not be ~/.mozbuild). Also, use os.path.join().

Comment 22

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 23

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Patch to make clang-format build statically

I built clang-format with:

$ configure --enable-libcpp --enable-cxx11 --enable-debug-symbols=no --enable-optimized --disable-libcpp

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8358166 [details] [diff] [review]
Add a clang-format-diff helper to mach;

Review of attachment 8358166 [details] [diff] [review]:
-----------------------------------------------------------------

I like the approach. Just some minor issues stand in the way of r+.

::: tools/mach_commands.py
@@ +326,5 @@
> +        plat = platform.system()
> +        if plat == "Windows":
> +            fmt += ".exe"
> +        elif plat != "Linux" or os.uname()[4] != 'x86_64':
> +            print "Unsupported platform: " + plat

Nit: Use print()

Please also print which platforms are supported so people don't scratch their heads.

@@ +340,5 @@
> +                return 0
> +
> +        except urllib2.HTTPError as e:
> +            print "HTTP error {0}: {1}".format(e.code, e.reason)
> +            return 0

0?

@@ +347,5 @@
> +        p1 = Popen(["hg", "diff", "-U0", "-r", "tip^", "--exclude", ".clang-format-ignore"], stdout=PIPE)
> +        p2 = Popen([sys.executable, clang_format_diff, "-p1", "-style=Mozilla"], stdin=p1.stdout)
> +        return p2.communicate()[0]
> +
> +    def locate_or_fetch(self, root):

Please add a comment here stating the temporary nature and intended long-term solution. Link to a bug if possible.

@@ +352,5 @@
> +        target = os.path.join(self._mach_context.state_dir, root)
> +        if not os.path.exists(target):
> +            site = "http://people.mozilla.org/~ajones/clang-format/"
> +            if self.prompt and raw_input("Download executables from {0} (yN)? ".format(site)).lower() != 'y':
> +                print "Download aborted"

You might want to state which executables. This message sounds scary as it is!

@@ +362,5 @@
> +            data = urllib2.urlopen(url=u, timeout=20).read()
> +            temp = target + ".tmp"
> +            f = open(temp, "wb")
> +            f.write(data)
> +            f.close()

with open(temp, 'wb') as fh:
  fh.write(data)

@@ +365,5 @@
> +            f.write(data)
> +            f.close()
> +            st = os.stat(temp)
> +            os.chmod(temp, st.st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
> +            os.rename(temp, target)

I'm pretty sure there is a function in the shutil module that can do this in one line.

@@ +367,5 @@
> +            st = os.stat(temp)
> +            os.chmod(temp, st.st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
> +            os.rename(temp, target)
> +        return target
> +   

Nit: trailing whitespace

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Add a clang-format-diff helper to mach;

Comment 26

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Gregory Szorc [:gps] from comment #24)
> > +        except urllib2.HTTPError as e:
> > +            print "HTTP error {0}: {1}".format(e.code, e.reason)
> > +            return 0
> 
> 0?

I don't know what the idiomatic python is supposed to be. I changed it to 'return' in the hope that is what you expect.

> @@ +365,5 @@
> > +            f.write(data)
> > +            f.close()
> > +            st = os.stat(temp)
> > +            os.chmod(temp, st.st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
> > +            os.rename(temp, target)
> 
> I'm pretty sure there is a function in the shutil module that can do this in
> one line.

I couldn't find anything helpful in shutil.

> @@ +367,5 @@
> > +            st = os.stat(temp)
> > +            os.chmod(temp, st.st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
> > +            os.rename(temp, target)
> > +        return target
> > +   
> 
> Nit: trailing whitespace

Spot the irony.

Comment 27

[Gregory Szorc [:gps]]

Comment on attachment 8362295 [details] [diff] [review]
Add a clang-format-diff helper to mach;

Review of attachment 8362295 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/mach_commands.py
@@ +312,5 @@
> +
> +@CommandProvider
> +class FormatProvider(MachCommandBase):
> +    @Command('clang-format', category='devenv', allow_all_args=True,
> +        description='Run clangformat on current changes')

clang-format

@@ +333,5 @@
> +            arch = os.uname()[4]
> +            if plat != "Linux" or arch != 'x86_64':
> +                print("Unsupported platform " + plat + "/" + arch +
> +                      ". Supported platforms are Windows/* and Linux/x86_64")
> +                return

The "0?" in the original review was "why 0?" This should be "return 1" to indicate non-success.

@@ +339,5 @@
> +        os.chdir(self.topsrcdir)
> +
> +        try:
> +            if not self.locate_or_fetch(fmt):
> +                return

return 1

@@ +342,5 @@
> +            if not self.locate_or_fetch(fmt):
> +                return
> +            clang_format_diff = self.locate_or_fetch(fmt_diff)
> +            if not clang_format_diff:
> +                return

return 1

@@ +346,5 @@
> +                return
> +
> +        except urllib2.HTTPError as e:
> +            print("HTTP error {0}: {1}".format(e.code, e.reason))
> +            return

return 1

@@ +349,5 @@
> +            print("HTTP error {0}: {1}".format(e.code, e.reason))
> +            return
> +
> +        from subprocess import Popen, PIPE
> +        p1 = Popen(["hg", "diff", "-U0", "-r", "tip^", "--exclude", ".clang-format-ignore"], stdout=PIPE)

Does this actually read the file correctly? Looking at Mercurial's documentation and source code, I don't think it does. But, according to `hg help patterns`, you can fix this by changing to:

--exclude listfile:.clang-format-ignore

@@ +359,5 @@
> +        if not os.path.exists(target):
> +            site = "http://people.mozilla.org/~ajones/clang-format/"
> +            if self.prompt and raw_input("Download clang-format executables from {0} (yN)? ".format(site)).lower() != 'y':
> +                print("Download aborted.")
> +                return

return 1?

@@ +364,5 @@
> +            self.prompt = 0
> +
> +            u = site + root
> +            print("Downloading {0} to {1}".format(u, target))
> +            data = urllib2.urlopen(url=u, timeout=20).read()

20s seems low for slow connections. What's wrong with the default timeout? Users can always hit ctrl-c.

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a171743fb50e

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a171743fb50e