Add presubmit check that runs dartfmt

Add presubmit check that runs dartfmt

BUG=
R=johnmccutchan@google.com, whesse@google.com, zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/409150339f9c33d2e7faf3c65a89166d7d4e4dc7

[Jacob, 2017-03-20 17:43:00]

jacobr@google.com changed reviewers:
+ johnmccutchan@google.com, whesse@google.com

[Jacob, 2017-03-20 17:43:01]

I can wait to submit this until the manual CLs to fixup dartfmt land.

Generated files that are committed to the SDK should have dartfmt run on them
before the files are committed. This should be a reasonable forcing function to
encourage that
to happen.
Users who want to ignore the formatter for now can upload
and commit with 
--ignore-presubmits

[Cutch, 2017-03-20 17:49:13]

lgtm

[Bill Hesse, 2017-03-20 17:50:00]

I'd like to get everyone to agree to this, and we may find out about some
categories of files, or some individual files, that may need to be excluded. 
Even if we don't know, yet, we should publicize that if someone has a problem
with a dart file that they don't want to format, for some reason, then they
should let us know, and use --bypass-hooks until we fix it.

So I would send out a message to dart-core-eng and dart-developers (if that is
what it is called), before landing this.

Otherwise, LGTM, though we may need to add a whitelist.

[Cutch, 2017-03-20 17:51:18]

On 2017/03/20 17:50:00, Bill Hesse wrote:
> I'd like to get everyone to agree to this, and we may find out about some
> categories of files, or some individual files, that may need to be excluded. 
> Even if we don't know, yet, we should publicize that if someone has a problem
> with a dart file that they don't want to format, for some reason, then they
> should let us know, and use --bypass-hooks until we fix it.
> 

If we can't use the dartfmt on our own code than we shouldn't ship it as part of
the SDK.

[zra, 2017-03-20 19:26:05]

zra@google.com changed reviewers:
+ zra@google.com

[zra, 2017-03-20 19:26:06]

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py#newcode28
PRESUBMIT.py:28: prebuilt_dartfmt = os.path.join(utils.CheckedInSdkPath(),
'bin', 'dartfmt')
The presubmit check should degrade gracefully if the this file doesn't exist. It
doesn't exist, e.g. in the Fuchsia tree.

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py#newcode34
PRESUBMIT.py:34: if os.system(cmd):
Could you rather use the presubmit API to run commands? See:
http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts for

[Jacob, 2017-03-20 19:59:07]

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py#newcode28
PRESUBMIT.py:28: prebuilt_dartfmt = os.path.join(utils.CheckedInSdkPath(),
'bin', 'dartfmt')
On 2017/03/20 19:26:06, zra wrote:
> The presubmit check should degrade gracefully if the this file doesn't exist.
It
> doesn't exist, e.g. in the Fuchsia tree.
Done. I now print a warning if the file doesn't exist.

https://codereview.chromium.org/2761653003/diff/20001/PRESUBMIT.py#newcode34
PRESUBMIT.py:34: if os.system(cmd):
On 2017/03/20 19:26:06, zra wrote:
> Could you rather use the presubmit API to run commands? See:
> http://dev.chromium.org/developers/how-tos/depottools/presubmit-scripts for

Done. Thanks for pointing out the existing presubmit API that does what I need.

[kevmoo, 2017-03-20 20:13:24]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2017-03-20 20:13:25]

Any concerns about dartfmt version?

Bob just landed fixed to dartfmt that will be rolled into 1.23 – I hope.

This is run on the dev machine, right? We don't have any kind of enforcement for
the version of dartfmt (SDK) on the dev machine.

Should we be using/running the version of dartfmt that is aligned w/ the
checkout of SDK?

[rmacnak, 2017-03-20 20:52:11]

rmacnak@google.com changed reviewers:
+ rmacnak@google.com

[rmacnak, 2017-03-20 20:52:12]

I've just noticed that dartfmt does not correctly preserve multitest
annotations, so we cannot use it as is for formatting tests.

[Jacob, 2017-03-20 21:17:55]

On 2017/03/20 20:52:12, rmacnak wrote:
> I've just noticed that dartfmt does not correctly preserve multitest
> annotations, so we cannot use it as is for formatting tests.

Good point. The multitest syntax currently breaks the formatter.
The formatter is technically correct that /// is a doc comment and
its treatment of doc comments is generally good. Instead I'll send a
CL to fix test.dart to allow //# as well as /// for multitest comments.
and send out a CL to cleanup all multitests to use //# instead of ///
At that point people should be able to check in multitest CLs without 
--bypass-hooks

[Jacob, 2017-03-21 19:29:44]

Tweaked presubmit so files that already had formatter deltas only generate
warning messages instead of errors.
Example test output from running locally:
Also tweaked regular error message so you can copy and paste to run dartfmt.

WARNING: 
tests/html/canvas_pixel_array_type_alias_test.dart has existing and possibly new
dartfmt issues

** Presubmit ERRORS **
Must run dartfmt on
/usr/local/google/home/jacobr/src/dart/git_14/sdk/example.dart

Must run dartfmt on
/usr/local/google/home/jacobr/src/dart/git_14/sdk/pkg/dev_compiler/web/web_command.dart

[zra, 2017-03-21 19:51:17]

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode33
PRESUBMIT.py:33: prebuilt_dartfmt = os.path.join(utils.CheckedInSdkPath(),
'bin', 'dartfmt')
Please add back the check that bails out if this doesn't exist.

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode39
PRESUBMIT.py:39: if os.system(cmd):
I think subprocess tends to be preferred over os.system

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode45
PRESUBMIT.py:45: f = open(temp_path, 'w')
with open(temp_path, 'w') as f:
  print(old_contents, file=f)

[Jacob, 2017-03-21 20:34:25]

Also cleaned up the error output slightly based on an offline suggestion from
nbosch.

Running presubmit upload checks ...
WARNING: tests/html/canvas_pixel_array_type_alias_test.dart has existing and
possibly new dartfmt issues

** Presubmit ERRORS **
File output does not match dartfmt.
Fix these issues with:
dartfmt -w \
/usr/local/google/home/jacobr/src/dart/git_14/sdk/example.dart \
/usr/local/google/home/jacobr/src/dart/git_14/sdk/pkg/dev_compiler/web/web_command.dart

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode33
PRESUBMIT.py:33: prebuilt_dartfmt = os.path.join(utils.CheckedInSdkPath(),
'bin', 'dartfmt')
On 2017/03/21 19:51:17, zra wrote:
> Please add back the check that bails out if this doesn't exist.

Done.

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode39
PRESUBMIT.py:39: if os.system(cmd):
On 2017/03/21 19:51:17, zra wrote:
> I think subprocess tends to be preferred over os.system

Done.

https://codereview.chromium.org/2761653003/diff/60001/PRESUBMIT.py#newcode45
PRESUBMIT.py:45: f = open(temp_path, 'w')
On 2017/03/21 19:51:17, zra wrote:
> with open(temp_path, 'w') as f:
>   print(old_contents, file=f)

Done.

[zra, 2017-03-21 20:47:49]

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py#newcode64
PRESUBMIT.py:64: os.close(fd)
It might also be a good idea to manually delete the file. The docs seem to
indicate that one should:
https://docs.python.org/2/library/tempfile.html#tempfile.mkstemp

Maybe it would be better to use tempfile.NamedTemporaryFile like:

with tempfile.NamedTemporaryFile() as temp:
  print(old_contents, file=temp)
  if HasFormatErrors(temp.name):
    ...

Also be sure to check that this all works on Windows before landing. Sometimes
stuff like this works differently there.

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py#newcode65
PRESUBMIT.py:65: except subprocess.CalledProcessError as e:
Can this still happen? It looks like HasFormatErrors is catching
CalledProcessError.

[Jacob, 2017-03-21 22:04:22]

verifying on a windows machine now

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py#newcode64
PRESUBMIT.py:64: os.close(fd)
On 2017/03/21 20:47:49, zra wrote:
> It might also be a good idea to manually delete the file. The docs seem to
> indicate that one should:
> https://docs.python.org/2/library/tempfile.html#tempfile.mkstemp
> 
> Maybe it would be better to use tempfile.NamedTemporaryFile like:
> 
> with tempfile.NamedTemporaryFile() as temp:
>   print(old_contents, file=temp)
>   if HasFormatErrors(temp.name):
>     ...
> 
> Also be sure to check that this all works on Windows before landing. Sometimes
> stuff like this works differently there.
Worked around a bug in how the formatter handles reading content from stdin so
switched to executing the formatter using stdin which eliminates the need for a
tmp file

https://codereview.chromium.org/2761653003/diff/80001/PRESUBMIT.py#newcode65
PRESUBMIT.py:65: except subprocess.CalledProcessError as e:
On 2017/03/21 20:47:49, zra wrote:
> Can this still happen? It looks like HasFormatErrors is catching
> CalledProcessError.

scm.GIT.Capture is throwing this error.

[zra, 2017-03-21 22:14:35]

lgtm

[Jacob, 2017-03-21 23:11:12]

Fixed behavior on windows.

[Jacob, 2017-03-21 23:18:52]

Description was changed from

==========
Add presubmit check that runs dartfmt

BUG=
==========

to

==========
Add presubmit check that runs dartfmt

BUG=
R=johnmccutchan@google.com, whesse@google.com, zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/409150339f9c33d2e7faf3c65a89166d7d4e4dc7
==========

[Jacob, 2017-03-21 23:18:54]

Committed patchset #12 (id:220001) manually as
409150339f9c33d2e7faf3c65a89166d7d4e4dc7 (presubmit successful).