1511093 - Various issues with format-source and clang-format

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, defect)

Description

[Steve Fink [:sfink] [:s:]]

I took a stab at using format-source, just to see what it's like, and ran into numerous issues. At the moment, I'm stuck. I could probably fix things to try to make more progress, but I'd like to log my issues so far. I should probably have filed separate bugs for each of these, but I don't want to put in the time just yet.

----

1. help message refers to -style=Mozilla which I think is no longer desired? At least, the newsgroup post did not contain that.

2. must be run from repo root

% hg format-source clang-format jsapi.cpp
abort: format-source must be run from repository root
(cd /home/sfink/src/mozilla)

3. weird glob pattern restriction:

% hg format-source clang-format js/src/jsapi.cpp
abort: format-source only supports explicit 'glob' patterns for now ('js/src/jsapi.cpp')
(maybe try with "glob:js/src/jsapi.cpp")

4. lacking detailed error message

% hg format-source clang-format glob:js/src/jsapi.cpp
abort: clang-format: mach exited with status 1

the actual error:

% mach clang-format -p js/src/jsapi.cpp
 0:01.51 Could not find artifacts for a toolchain build named `linux64-clang-tidy`. Local commits and other changes in your checkout may cause this error. Try updating to a fresh checkout of mozilla-central to use artifact builds.

5. Exception: Couldn't find taskgraph configuration: taskcluster/ci/config.yml

% mach clang-format -p js/src/jsapi.cpp
...
Exception: Couldn't find taskgraph configuration: taskcluster/ci/config.yml
...

(<repo-root aka pwd>/taskcluster/ci/config.yml exists)

It's looking for /home/sfink/.mozbuild/clang-tools/taskcluster/ci/config.yml. It apparently cds to the wrong directory.

----

In addition, some things were unclear to me. The format-source extension seems to be a more general thing, but the description doesn't really explain what's going on. It seems to be something like a general tool to record formatting changes for use with later rebases or merges? "help dealing with code source reformating[sic]" doesn't say very much, and just from the name my initial naive assumption would be that it just reformats source in the current working tree or something. Clearly that's not right, though. Then the command help says "format source file using a registered tools". I'm not sure if the "a" shouldn't be there, or "tools" should be singular, but either way it seems to be directly saying that it *is* simply reformatting working directory source. I could probably suggest better wording once I myself figure out what the heck this thing does.

This sentence from https://docs.google.com/document/d/13AwAsvKMhH0mflDlfatBqn6LmZHiQih76oxM4zfrPl4/edit seems illuminating: "Now you will only have to rebase your patch against the formatted repo, the extension will be triggered automatically during the merge/rebase." Even better is Ehsan's post:

"That's exactly what the hg-formatsource extension does for you, it reformats your local changes on the fly as the rebase is in progress, so you will get no collisions."

Ok... so I *think* this sets things up so that when rebasing a patch based on a pre-formatted tree, the patch (or the result of the patch?) will automatically be formatted before performing the 3-way merge? And perhaps it does that regardless of what your patch is based on; it will just unconditionally do clang-format prior to the 3-way merge so once you have this set up you don't need to bother with sticking to the accepted whitespace style at all?

Comment 1

[Andi [:andi]]

For the issue with 'mach clang-format' please see bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1511048
Also we've just pushed a minor modification to 'vcs' so please run again 'mach bootstrap'

In general you don't need to use 'hg format-source clang-format'. This should be used when we add a new folder that is not yet listed as being formatted. 
Just as you said format-source is a tool that tracks paths where formatted code resides and when doing the merge it will automatically format your local changes and try to merge with the existing ones from the repo.

Comment 2

[Andi [:andi]]

If you need help with this please need-info me directly or ping me on irc.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Steve Fink [:sfink] [:s:] from comment #0)
> 5. Exception: Couldn't find taskgraph configuration:
> taskcluster/ci/config.yml

Duplicate of bug 1511047

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> In general you don't need to use 'hg format-source clang-format'. This
> should be used when we add a new folder that is not yet listed as being
> formatted. 

Ok, that was not clear from either the dev-platform thread or the extension help message. I would vote to rename the command to register-source-formatter with an alias of format-source. Then the help message: "This command run TOOL on FILES and record this information in a commit to help with future merge." This sounds very much like git rerere (which, to be honest, I've never really understood) in that it records merge results so you don't have to keep re-specifying them. But in actuality, it sounds like it does not do that at all, but rather something like "This command registers TOOL to be run whenever a file matching FILES is modified. The registration is stored in a commit and is used by future merges and rebases."? Oh... but by skimming the source, it looks like it also runs the tool immediately? I guess I don't understand the model well enough to know why it does an immediate reformatting; it seems like it would be simpler if it only did the registration part.

Ok, I'm not sure what the help should say.

> Just as you said format-source is a tool that tracks paths where formatted
> code resides and when doing the merge it will automatically format your
> local changes and try to merge with the existing ones from the repo.

But it also does an immediate reformat?

Comment 4

[Andi [:andi]]

Yes it does an immediate format.

Comment 5

[(no longer active)]

(In reply to Steve Fink [:sfink] [:s:] from comment #3)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #1)
> > In general you don't need to use 'hg format-source clang-format'. This
> > should be used when we add a new folder that is not yet listed as being
> > formatted. 
> 
> Ok, that was not clear from either the dev-platform thread or the extension
> help message. I would vote to rename the command to
> register-source-formatter with an alias of format-source. Then the help
> message: "This command run TOOL on FILES and record this information in a
> commit to help with future merge."

I think this is a good idea.  Maybe we should also add something to the help saying "you probably don't want to be running this command directly" or some such...

Comment 6

[Steve Fink [:sfink] [:s:]]

From talking to people, it seems like people expect hg format-source to do source formatting. I'll upload a patch I made that updates the comments to something that makes sense to me. I'm not necessarily proposing that it be landed as-is; I didn't verify that everything I describe in it is actually what is going on.

One thing in particular -- the current docs make a point of saying that the path to the formatting tool is stored globally, not in the repo, for security reasons. Is that actually true? My confusion is that there are 3 possible places to put it: committed to the repo, in the repo's .hg/hgrc file, or in the global .hgrc file. I would assume that only the first would be a problem? It seems to me that it would make sense to have repo-specific settings in a repo's .hg/hgrc file.

Anyway, these edits are just a suggestion.

One thing that this does not cover that I ran into recently: the extension will *not* reformat rebased files if the changes you are rebasing over do not touch that file. I attempted to reformat my patch stack by rebasing to a commit past the big style change (because I was struggling with getting clang-tidy installed when using the original revision), then setting up the extension properly, updating to the latest mozilla-inbound, and rebasing on top of that. Despite all of my changesets being updated, the formatting ended up wrong. In debugging it, I learned that it's not enough for a file to be part of a rebase; it has to have changes on both sides of the merge, since otherwise the internal logic will just grab my modified version and not call filemerge at all. It makes sense in retrospect, but was unexpected.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Comment suggestions for format-source extension

I guess I should be using phabricator for this, but I realized I wasn't sure how to associate it with the right bug and didn't want to bother to figure that out right now. (It probably would have just worked if I put the bug in the commit message?)

Comment 8

[Andi [:andi]]

Comment on attachment 9029664 [details] [diff] [review]
Comment suggestions for format-source extension

Steve thanks for doing this, all your modifications look reasonable and well sorted out, I think we should accept these modifications, so if not to much of a deal for you could you please use phabricator? Just as you said in order to associate the patch submitted to differential with this bug, just but the bug number in the title.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1511093 - Minor spelling and wording fixes

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/68f59905d10e
Minor spelling and wording fixes r=andi