Depot tools: use the clang-format binaries that are now included

Depot tools: use the clang-format binaries that are now included
as part of the Chromium checkout.

This follows the approach used by gn.

Changes include:
 - in-the-PATH clang-format trampoline scripts
 - clang_format.py, which finds clang-format binaries inside of Chrome
 - Hook 'git cl format' to the new binaries and scripts
 - Rearrange some code, for reuse between clang_format.py and gn.py

BUG=240309
TEST=presubmits (one failure on mac, but it fails on a clean checkout too)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=245074

[ncarter (slow), 2014-01-11 02:00:38]

This is ready for a review (though obviously we will land the chrome part
first).

My main concern is what happens to existing users -- depot_tools will tend to
run ahead of the chrome checkouts.

Three possible migration strategies:

[1] Just break things for now, with an apologetic warning.
[2] Try to fall back to the system-installed clang-format. This is made tricky
because we're adding new scripts to depot_tools called 'clang-format' which are
intended to shadow the system binary. So we'd have to either manually traverse
$PATH, while knowing to disqualify hits in depot_tools, or else, hard code an
absolute search list.
[3] Do [1] but wait a week or two first, to lessen the impact on people's work.
[4] Do [2] but make it more robust/simple by eliminating the 'clang-format'
script, so there's no shadowing problem, but people can't run clang-format from
the command line.

I did [1] but let me know what you think.

[ncarter (slow), 2014-01-11 21:01:46]

On 2014/01/11 02:00:38, ncarter wrote:
> This is ready for a review (though obviously we will land the chrome part
> first).
> 
> My main concern is what happens to existing users -- depot_tools will tend to
> run ahead of the chrome checkouts.
> 
> Three possible migration strategies:
> 
> [1] Just break things for now, with an apologetic warning.
> [2] Try to fall back to the system-installed clang-format. This is made tricky
> because we're adding new scripts to depot_tools called 'clang-format' which
are
> intended to shadow the system binary. So we'd have to either manually traverse
> $PATH, while knowing to disqualify hits in depot_tools, or else, hard code an
> absolute search list.
> [3] Do [1] but wait a week or two first, to lessen the impact on people's
work.
> [4] Do [2] but make it more robust/simple by eliminating the 'clang-format'
> script, so there's no shadowing problem, but people can't run clang-format
from
> the command line.
> 
> I did [1] but let me know what you think.

Thinking over this last night I think I might have overcomplicated the problem.
The script already hardcoded /usr/lib/clang-format-diff.py, so I don't think
it's much of a stretch to also hardcode a /usr/bin/clang-format as a fallback.
I'll switch to this.

[Nico, 2014-01-11 21:05:58]

On Sat, Jan 11, 2014 at 1:01 PM, <nick@chromium.org> wrote:

> On 2014/01/11 02:00:38, ncarter wrote:
>
>> This is ready for a review (though obviously we will land the chrome part
>> first).
>>
>
>  My main concern is what happens to existing users -- depot_tools will
>> tend to
>> run ahead of the chrome checkouts.
>>
>
>  Three possible migration strategies:
>>
>
>  [1] Just break things for now, with an apologetic warning.
>> [2] Try to fall back to the system-installed clang-format. This is made
>> tricky
>> because we're adding new scripts to depot_tools called 'clang-format'
>> which
>>
> are
>
>> intended to shadow the system binary. So we'd have to either manually
>> traverse
>> $PATH, while knowing to disqualify hits in depot_tools, or else, hard
>> code an
>> absolute search list.
>> [3] Do [1] but wait a week or two first, to lessen the impact on people's
>>
> work.
>
>> [4] Do [2] but make it more robust/simple by eliminating the
>> 'clang-format'
>> script, so there's no shadowing problem, but people can't run clang-format
>>
> from
>
>> the command line.
>>
>
>  I did [1] but let me know what you think.
>>
>
> Thinking over this last night I think I might have overcomplicated the
> problem.
> The script already hardcoded /usr/lib/clang-format-diff.py, so I don't
> think
> it's much of a stretch to also hardcode a /usr/bin/clang-format as a
> fallback.
> I'll switch to this.
>

I'd prefer if we don't mention /usr/bin/clang-format in this script if
possible. (Haven't looked at your patch yet through – trying to get the
tree green so the dependency can land)


>
> https://codereview.chromium.org/134313007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-11 23:33:07]

lgtm

I think this looks pretty good as-is. (Before announcing this more widely, we
might want to try adding a .clang-format file to blink again.)

[ncarter (slow), 2014-01-13 20:23:43]

+maruel, for OWNERS
+brettw, FYI, some gn script refactoring

[M-A Ruel, 2014-01-13 20:42:06]

I'll let Robbie do the actual review on my behalf.

[brettw, 2014-01-13 20:44:03]

okey dokey!

[ncarter (slow), 2014-01-14 23:32:08]

iannucci: ping

[enne (OOO), 2014-01-15 00:59:47]

https://codereview.chromium.org/134313007/diff/140001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/134313007/diff/140001/git_cl.py#newcode2365
git_cl.py:2365: cmd = [sys.executable, script, '-p0', '-style', 'Chromium']
Are you missing an '-i' here?

[ncarter (slow), 2014-01-15 01:18:36]

Fixed and retested.

https://codereview.chromium.org/134313007/diff/140001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/134313007/diff/140001/git_cl.py#newcode2365
git_cl.py:2365: cmd = [sys.executable, script, '-p0', '-style', 'Chromium']
On 2014/01/15 00:59:47, enne wrote:
> Are you missing an '-i' here?

Oh my! You're right! Shameful last-minute cleanup.

[ncarter (slow), 2014-01-15 01:18:37]

Fixed and retested.

https://codereview.chromium.org/134313007/diff/140001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/134313007/diff/140001/git_cl.py#newcode2365
git_cl.py:2365: cmd = [sys.executable, script, '-p0', '-style', 'Chromium']
On 2014/01/15 00:59:47, enne wrote:
> Are you missing an '-i' here?

Oh my! You're right! Shameful last-minute cleanup.

[Nico, 2014-01-15 17:32:28]

iannucci: ping ping ping

[iannucci, 2014-01-15 20:29:31]

My only objection is the amount of chromium-specific code which is introduced
here to depot_tools (and the fact that depot_tools will now ship with a broken
command (unless you use depot_tools to pull chromium)).

However, I don't want to hang up the introduction of clang-format on that though
(because obviously 90%+ of depot_tools users are chromium-source users), and
since depot_tools doesn't really have a built-in way to forward execution to
scripts in checkouts, I'll give it a pass.

If someone has good ideas for how to remove the chromium stuff to make `git cl
format` work with only depot_tools as the single manual dependency, that would
be pretty great.

Or even just a generic mechanism for git-cl to look for subcommands in the
current checkout, git-style (i.e. so that git-cl-format could live in
chromium/src or something).

lgtm

https://codereview.chromium.org/134313007/diff/220001/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/134313007/diff/220001/gclient_utils.py#newcod...
gclient_utils.py:566: if sys.platform in ('cygwin', 'win32'):
sys.platform.startswith(('cygwin', 'win'))

[ncarter (slow), 2014-01-15 23:20:26]

Updated. I'm testing against a fresh cygwin checkout now. Do I submit via cq or
dcommit?

[ncarter (slow), 2014-01-15 23:21:15]

On 2014/01/15 23:20:26, ncarter wrote:
> Updated. I'm testing against a fresh cygwin checkout now. Do I submit via cq
or
> dcommit?

Answered my own question. Will use the cq.

[commit-bot: I haz the power, 2014-01-15 23:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/134313007/310001

[commit-bot: I haz the power, 2014-01-15 23:52:23]

Presubmit check for 134313007-310001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Pylint (81 files) (29.50s) failed
************* Module toolchain2013
W0602: 40,2:TempDir: Using global for 'g_temp_dirs' but no assignment is done
W0622: 80,33:Download: Redefining built-in 'file'
W0612:184,14:CopyToFinalLocation: Unused variable 'dirnames'
W0612:268,11:main: Unused variable 'args'


tests/gclient_utils_test.py (0.15s) failed
.....['GetExeSuffix', 'GetMacWinOrLinux']
F..
======================================================================
FAIL: testMembersChanged (__main__.GclientUtilsUnittest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/b/commit-queue/workdir/tools/depot_tools/third_party/pymox/mox.py",
line 1618, in new_method
    func(self, *args, **kwargs)
  File "tests/gclient_utils_test.py", line 45, in testMembersChanged
    self.compareMembers(gclient_utils, members)
  File "/b/commit-queue/workdir/tools/depot_tools/testing_support/super_mox.py",
line 78, in compareMembers
    self.assertEqual(actual_members, expected_members)
AssertionError: Lists differ: ['Annotated', 'AutoFlush', 'Ch... != ['Annotated',
'AutoFlush', 'Ch...

First differing element 15:
GetExeSuffix
GetGClientRootAndEntries

First list contains 2 additional elements.
First extra element 50:
time

Diff is 979 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 8 tests in 0.016s

FAILED (failures=1)


Presubmit checks took 123.8s to calculate.

[commit-bot: I haz the power, 2014-01-16 02:28:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/134313007/370001

[commit-bot: I haz the power, 2014-01-16 02:30:45]

Presubmit check for 134313007-370001 failed and returned exit status 1.

Running presubmit commit checks ...
Checking out rietveld...
Running save-description-on-failure.sh
Running push-basic.sh
Running upstream.sh
Running submit-from-new-dir.sh
Running abandon.sh
Running submodule-merge-test.sh
Running upload-local-tracking-branch.sh
Running hooks.sh
Running post-dcommit-hook-test.sh
Running upload-stale.sh
Running patch.sh
Running basic.sh

** Presubmit ERRORS **
Pylint (81 files) (25.61s) failed
************* Module toolchain2013
W0602: 40,2:TempDir: Using global for 'g_temp_dirs' but no assignment is done
W0622: 80,33:Download: Redefining built-in 'file'
W0612:184,14:CopyToFinalLocation: Unused variable 'dirnames'
W0612:268,11:main: Unused variable 'args'


Presubmit checks took 152.7s to calculate.

[commit-bot: I haz the power, 2014-01-16 02:43:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nick@chromium.org/134313007/370001

[commit-bot: I haz the power, 2014-01-16 02:44:43]

Change committed as 245074