Run dartfmt when invoking git cl format.

Run dartfmt when invoking git cl format.

If the repository has third_party/dart-sdk/ unpacked, use that to
format dart files modified in the current patch.

BUG=459376
R=maruel@chromium.org, zra@google.com

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=294179

[Elliot Glaysher, 2015-02-18 20:41:41]

erg@chromium.org changed reviewers:
+ enne@chromium.org, zra@google.com

[Elliot Glaysher, 2015-02-18 20:41:42]

enne: I have a specific question about something you wrote.
zra: review

https://codereview.chromium.org/933383002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/933383002/diff/1/git_cl.py#newcode2997
git_cl.py:2997: return 2
enne: you added the above two lines to return a value to the cmd prompt when a
dry run is done and there's output. What motivated this? Should I do something
analogous in the block below?

[enne (OOO), 2015-02-18 20:56:46]

https://codereview.chromium.org/933383002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/933383002/diff/1/git_cl.py#newcode2997
git_cl.py:2997: return 2
On 2015/02/18 20:41:42, Elliot Glaysher wrote:
> enne: you added the above two lines to return a value to the cmd prompt when a
> dry run is done and there's output. What motivated this? Should I do something
> analogous in the block below?

Yeah, the "return 2" is to indicate to CheckPatchFormatted that "git cl format"
was not a no-op and that there were formatting changes.

See:
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/pres...

[Elliot Glaysher, 2015-02-18 21:16:17]

https://codereview.chromium.org/933383002/diff/1/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/933383002/diff/1/git_cl.py#newcode2997
git_cl.py:2997: return 2
On 2015/02/18 20:56:46, enne wrote:
> On 2015/02/18 20:41:42, Elliot Glaysher wrote:
> > enne: you added the above two lines to return a value to the cmd prompt when
a
> > dry run is done and there's output. What motivated this? Should I do
something
> > analogous in the block below?
> 
> Yeah, the "return 2" is to indicate to CheckPatchFormatted that "git cl
format"
> was not a no-op and that there were formatting changes.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/pres...

TYVM. I've modified this so that unformatted dart files cause this function to
return 2, and I've modified the mojo repository's presubmit scripts to run
CheckPatchFormatted, verifying that this works. (That will be landed
separately.)

[zra, 2015-02-18 22:05:15]

Am I reading this correctly that if you have Dart code, but don't have the Dart
SDK checked out under third_party, presubmit checks will always fail? Is that a
problem? Also, do you have  a reviewer from the depot_tools team?

[Elliot Glaysher, 2015-02-18 22:35:27]

On 2015/02/18 22:05:15, zra wrote:
> Am I reading this correctly that if you have Dart code, but don't have the
Dart
> SDK checked out under third_party, presubmit checks will always fail? Is that
a
> problem?

Yes, it is a problem. I've changed this so that it will print an error message.
This should keep uploading dart files from breaking in the chromium repo (and in
the modular repo before I land the equivalent third_party deps script there).

> Also, do you have  a reviewer from the depot_tools team?

Not yet. I wanted a preliminary review before that.

[zra, 2015-02-18 22:42:36]

lgtm

[Elliot Glaysher, 2015-02-18 22:46:12]

erg@chromium.org changed reviewers:
+ iannucci@chromium.org

[Elliot Glaysher, 2015-02-18 22:46:12]

iannucci: owners review

Context: At least the mojo repository (and soon other not-the-main-chromium
repos) need to have the dart sdk for some build tools. I'd like to also have
git-cl-format do the right thing when it's there.

[Elliot Glaysher, 2015-02-19 20:58:03]

erg@chromium.org changed reviewers:
+ maruel@google.com

[Elliot Glaysher, 2015-02-19 20:58:03]

+maruel: owners review since Robbie seems busy.

[Elliot Glaysher, 2015-02-23 18:33:37]

On 2015/02/19 20:58:03, Elliot Glaysher wrote:
> +maruel: owners review since Robbie seems busy.

Ping

[M-A Ruel, 2015-02-23 18:35:31]

On 2015/02/23 18:33:37, Elliot Glaysher wrote:
> On 2015/02/19 20:58:03, Elliot Glaysher wrote:
> > +maruel: owners review since Robbie seems busy.
> 
> Ping

Why isn't dart_format.py inside the dart repository?

[Elliot Glaysher, 2015-02-23 18:37:43]

On 2015/02/23 18:35:31, M-A Ruel wrote:
> On 2015/02/23 18:33:37, Elliot Glaysher wrote:
> > On 2015/02/19 20:58:03, Elliot Glaysher wrote:
> > > +maruel: owners review since Robbie seems busy.
> > 
> > Ping
> 
> Why isn't dart_format.py inside the dart repository?

Why would it be? clang_format.py is in depot_tools, and dart_format.py is a
simplified version of clang_format.py, except customized to call out to dartfmt
instead of clang-format.

[M-A Ruel, 2015-02-23 18:40:02]

On 2015/02/23 18:37:43, Elliot Glaysher wrote:
> On 2015/02/23 18:35:31, M-A Ruel wrote:
> > On 2015/02/23 18:33:37, Elliot Glaysher wrote:
> > > On 2015/02/19 20:58:03, Elliot Glaysher wrote:
> > > > +maruel: owners review since Robbie seems busy.
> > > 
> > > Ping
> > 
> > Why isn't dart_format.py inside the dart repository?
> 
> Why would it be? clang_format.py is in depot_tools, and dart_format.py is a
> simplified version of clang_format.py, except customized to call out to
dartfmt
> instead of clang-format.

The only reason clang_format is here is because it's used in multiple
repositories. Will dart_format be used by more than one repository?

[M-A Ruel, 2015-02-23 18:47:25]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-02-23 18:47:26]

stylistic nits.

I think there's a layering issue here but I guess the ship has sunk already.

https://codereview.chromium.org/933383002/diff/40001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode2896
git_cl.py:2896: 
2 empty lines between file level symbols.

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode2906
git_cl.py:2906: diff_cmd += [os.path.join(arg, '*' + ext) for ext in extensions]
diff_cmd.extend(os.path.join(arg, '*' + ext) for ext in extensions)

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode2912
git_cl.py:2912: diff_cmd += ['*' + ext for ext in extensions]
diff_cmd.extend('*' + ext for ext in extensions)

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode2915
git_cl.py:2915: 
2 empty lines

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode3012
git_cl.py:3012: command = [dartfmt]
command = [dart_format.FindDartFmtToolInChromiumTree()]

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode3016
git_cl.py:3016: command.extend(files)
command.extend(dart_diff_output.splitlines())

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode3019
git_cl.py:3019: if opts.dry_run and len(stdout) > 0:
if opts.dry_run and stdout:

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode3021
git_cl.py:3021: except dart_format.NotFoundError, e:
except dart_format.NotFoundError as e:

https://codereview.chromium.org/933383002/diff/40001/git_cl.py#newcode3022
git_cl.py:3022: print ("Unable to check dart code formatting. Dart SDK is not in
" +
single quotes

[Elliot Glaysher, 2015-02-23 18:55:34]

New patchsets have been uploaded after l-g-t-m from zra@google.com

[Elliot Glaysher, 2015-02-23 18:56:29]

ptal

[M-A Ruel, 2015-02-23 19:03:56]

lgtm

https://codereview.chromium.org/933383002/diff/60001/dart_format.py
File dart_format.py (right):

https://codereview.chromium.org/933383002/diff/60001/dart_format.py#newcode13
dart_format.py:13: import gclient_utils
In general, import stdlib first, only after module local imports, e.g.

import os
import subprocess
import sys

import gclient_utils


Class NotFoundError(Exception):
...

[Elliot Glaysher, 2015-02-23 19:07:59]

New patchsets have been uploaded after l-g-t-m from maruel@chromium.org

[Elliot Glaysher, 2015-02-23 19:08:31]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:08:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933383002/80001

[commit-bot: I haz the power, 2015-02-23 19:08:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 19:09:00]

Failed to apply patch for depot_tools/dart_format.py:
While running svn add depot_tools/dart_format.py --force --config-dir
/b/infra_internal/commit_queue/subversion_config --non-interactive;
  patching file dart_format.py
  svn: warning: W155010:
'/b/infra_internal/commit_queue/workdir/tools/depot_tools/dart_format.py' not
found
  svn: E200009: Could not add all targets because some targets don't exist
  svn: E200009: Illegal target for the requested operation

Patch:   N   depot_tools/dart_format.py
Index: dart_format.py
diff --git depot_tools/dart_format.py depot_tools/dart_format.py
new file mode 100755
index
0000000000000000000000000000000000000000..ee07efe2e3c8141dea679b1fbaa1ed0d9c9e530c
--- /dev/null
+++ b/depot_tools/dart_format.py
@@ -0,0 +1,58 @@
+#!/usr/bin/python
+# Copyright 2015 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Redirects to the version of dartfmt checked into a gclient repo.
+
+dartfmt binaries are pulled down during gclient sync in the mojo repo.
+
+This tool is named dart_format.py instead of dartfmt to parallel
+clang_format.py, which is in this same repository."""
+
+import os
+import subprocess
+import sys
+
+import gclient_utils
+
+class NotFoundError(Exception):
+  """A file could not be found."""
+  def __init__(self, e):
+    Exception.__init__(self,
+        'Problem while looking for dartfmt in Chromium source tree:\n'
+        '  %s' % e)
+
+
+def FindDartFmtToolInChromiumTree():
+  """Return a path to the dartfmt executable, or die trying."""
+  primary_solution_path = gclient_utils.GetPrimarySolutionPath()
+  if not primary_solution_path:
+    raise NotFoundError(
+        'Could not find checkout in any parent of the current path.')
+
+  dartfmt_path = os.path.join(primary_solution_path, 'third_party', 'dart-sdk',
+                              'dart-sdk', 'bin', 'dartfmt')
+  if not os.path.exists(dartfmt_path):
+    raise NotFoundError('File does not exist: %s' % dartfmt_path)
+  return dartfmt_path
+
+
+def main(args):
+  try:
+    tool = FindDartFmtToolInChromiumTree()
+  except NotFoundError, e:
+    print >> sys.stderr, e
+    sys.exit(1)
+
+  # Add some visibility to --help showing where the tool lives, since this
+  # redirection can be a little opaque.
+  help_syntax = ('-h', '--help', '-help', '-help-list', '--help-list')
+  if any(match in args for match in help_syntax):
+    print '\nDepot tools redirects you to the dartfmt at:\n    %s\n' % tool
+
+  return subprocess.call([tool] + sys.argv[1:])
+
+
+if __name__ == '__main__':
+  sys.exit(main(sys.argv))

[Elliot Glaysher, 2015-02-23 20:30:11]

Committed patchset #5 (id:80001) manually as 294179.