SDK build bits for dartfmt.

R=danrubel@google.com, kustermann@google.com, ricow@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32512

[pquitslund, 2013-11-19 18:44:57]

A work in progress but in case I have a long way to go I figured I'd open this
up to comments early...

FWIW: I don't plan to land this for a while as the formatter is not ready for
prime time just yet.  I'd like to get this up to snuff and staged for when it
is.

Thanks in advance for any pointers!

[ahe, 2013-11-19 18:55:27]

Who makes the decision that we need to have this tool in the SDK?

As far as I'm concerned, formatting source code doesn't seem like something that
is needed in the SDK.

What is the plan for testing this component?

Who will be responsible for the long term maintenance of this tool? Is this a
demonstration of technology, or is this a real project with a tech-lead?

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp...
utils/dartfmt/dartfmt.gyp:20: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg"])',
Where are the sources for dartfmt?

[pquitslund, 2013-11-19 19:37:26]

On 2013/11/19 18:55:27, ahe wrote:
> Who makes the decision that we need to have this tool in the SDK?
> 
> As far as I'm concerned, formatting source code doesn't seem like something
that
> is needed in the SDK.

I think we clarified this over IM but I should pop up and say I'm sorry that I
didn't give you more context.  In retrospect, I see how this CL could appear to
be coming entirely from left field.  Sorry for that!

> 
> What is the plan for testing this component?

Some rudimentary tests are in place here:

pkg/analyzer/test/services/

Many more to come. :)

> 
> Who will be responsible for the long term maintenance of this tool? Is this a
> demonstration of technology, or is this a real project with a tech-lead?

It's a real project and will be maintained by the editor team.  First and
foremost it's a command-line service that is entirely IDE-agnostic.  In short
order, the editor will call out to it for all of it's formatting needs.  (And to
your question over IM, there will be no redundancy at all.)

> https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp
> File utils/dartfmt/dartfmt.gyp (right):
> 
>
https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp...
> utils/dartfmt/dartfmt.gyp:20: '<!@(["python", "../../tools/list_files.py",
> "\\.dart$", "../../pkg"])',
> Where are the sources for dartfmt?

In the analyzer package directory.  Specifically:

pkg/analyzer/lib/ and 
pkg/analyzer/bin/

[ricow1, 2013-12-06 08:27:53]

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp...
utils/dartfmt/dartfmt.gyp:20: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg"])',
you need to add  '<(SHARED_INTERMEDIATE_DIR)/packages.stamp', as an input as
well, otherwise you are not guaranteed that it has been created first (i.e.,
that packages has been symlinked to the out dir)
Additionally, after doing that, you should not need to list the files in
../../pkg as input, that is implicit through the stamp file
Peter: After he has packages.stamp as input that should be enough to also
guarantee that he gets rebuild when the actual dartfmt source changes, you could
make it explicit here by listing ../../pkg/analyzer though (otherwise this could
look like black magic)

[kustermann, 2013-12-06 08:47:08]

I don't like the fact that we duplicate the sdk/bin/* files.

https://codereview.chromium.org/63363007/diff/30001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/63363007/diff/30001/dart.gyp#newcode53
dart.gyp:53: 'analyzer',
Why not depending on 'dartfmt' defined in this file (as is done with
'analyzer')?

[pquitslund, 2013-12-06 17:55:42]

Thanks for the input!

Tweaks applied.  Take a fresh look and let me know if I missed anything.

Cheers!

https://codereview.chromium.org/63363007/diff/30001/dart.gyp
File dart.gyp (right):

https://codereview.chromium.org/63363007/diff/30001/dart.gyp#newcode53
dart.gyp:53: 'analyzer',
On 2013/12/06 08:47:08, kustermann wrote:
> Why not depending on 'dartfmt' defined in this file (as is done with
> 'analyzer')?

I was following the example of dart2js and pub.  Updated now though.

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/30001/utils/dartfmt/dartfmt.gyp...
utils/dartfmt/dartfmt.gyp:20: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg"])',
On 2013/12/06 08:27:53, ricow1 wrote:
> you need to add  '<(SHARED_INTERMEDIATE_DIR)/packages.stamp', as an input as
> well, otherwise you are not guaranteed that it has been created first (i.e.,
> that packages has been symlinked to the out dir)
> Additionally, after doing that, you should not need to list the files in
> ../../pkg as input, that is implicit through the stamp file
> Peter: After he has packages.stamp as input that should be enough to also
> guarantee that he gets rebuild when the actual dartfmt source changes, you
could
> make it explicit here by listing ../../pkg/analyzer though (otherwise this
could
> look like black magic)

Excellent.  Done.  Thanks!

[kustermann, 2013-12-09 08:48:44]

I'm not sure how you test this, but I ran dartfmt on
tools/testing/dart/test_suite.dart and it makes strange formatting decisions:

--- snip ---
@@ -1945,7 +1945,7 @@
    static void add(Set<Expectation> expectations) {
     bool containsFail = expectations.any(
-        (expectation) => expectation.canBeOutcomeOf(Expectation.FAIL));
+    (expectation) => expectation.canBeOutcomeOf(Expectation.FAIL));
     ++total;
     if (expectations.contains(Expectation.SKIP)) {
       ++skipped;
@@ -1955,23 +1955,23 @@
     } else {
       // Counts the number of flaky tests.
       if (expectations.contains(Expectation.PASS) &&
-          containsFail &&
-          !expectations.contains(Expectation.CRASH) &&
-          !expectations.contains(Expectation.OK)) {
+      containsFail &&
+      !expectations.contains(Expectation.CRASH) &&
+      !expectations.contains(Expectation.OK)) {
         ++noCrash;
       }
       if (expectations.contains(Expectation.PASS) && expectations.length == 1)
{
         ++pass;
       }
       if (expectations.containsAll([Expectation.FAIL, Expectation.OK]) &&
-          expectations.length == 2) {
+      expectations.length == 2) {
         ++failOk;
       }
       if (containsFail && expectations.length == 1) {
         ++fail;
       }
       if (expectations.contains(Expectation.CRASH) &&
-          expectations.length == 1) {
+      expectations.length == 1) {
         ++crash;
       }
       if (expectations.contains(Expectation.TIMEOUT)) 

--- snip-end ---

I'd prefer if we don't ship dartfmt before we have a 
- an official unambiguous style guide (we do have [0], but I don't know if that
is "official")
- a test suite covering all rules in the style guide
- dartfmt is passing that test suite (> 95% of the test cases).

Once we have this achieved, we can run the formatter on all dart files in the
dart repository, see if we are satisfied with the result, if not, change the
rules in the style guide and iterate.

We can't use dartfmt to format our testing scripts for example, because the
indentation is different from how we format our code (as you can see above) [I
assume that's the same for dart2js ...].
If we can't use it for our code, our users probably shouldn't either?

[0] https://www.dartlang.org/articles/style-guide/

Take it as constructive feedback -- the decision when we ship dartfmt as part of
the SDK is not mine.

https://codereview.chromium.org/63363007/diff/110001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/110001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:29: '../../pkg/analyzer/bin/formatter.dart',
This command is building a snapshot of .../formatter.dart. A snapshot of a dart
application contains *all* dart code (which includes imported packages).

Therefore you need to add the correct package root here (so the VM can find the
imported packages when creating the snapshot). Something like this:
          'action': [
            '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)dart<(EXECUTABLE_SUFFIX)',
            '--snapshot=<(SHARED_INTERMEDIATE_DIR)/dartfmt.dart.snapshot',
            '--package-root=<(PRODUCT_DIR)/packages/',
            '../../pkg/analyzer/bin/formatter.dart',
          ],

[pquitslund, 2013-12-09 23:37:50]

Thanks Martin!

On 2013/12/09 08:48:44, kustermann wrote:
> I'm not sure how you test this, but I ran dartfmt on
> tools/testing/dart/test_suite.dart and it makes strange formatting decisions:
> 
> --- snip ---
> @@ -1945,7 +1945,7 @@
>     static void add(Set<Expectation> expectations) {
>      bool containsFail = expectations.any(
> -        (expectation) => expectation.canBeOutcomeOf(Expectation.FAIL));
> +    (expectation) => expectation.canBeOutcomeOf(Expectation.FAIL));
>      ++total;
>      if (expectations.contains(Expectation.SKIP)) {
>        ++skipped;
> @@ -1955,23 +1955,23 @@
>      } else {
>        // Counts the number of flaky tests.
>        if (expectations.contains(Expectation.PASS) &&
> -          containsFail &&
> -          !expectations.contains(Expectation.CRASH) &&
> -          !expectations.contains(Expectation.OK)) {
> +      containsFail &&
> +      !expectations.contains(Expectation.CRASH) &&
> +      !expectations.contains(Expectation.OK)) {
>          ++noCrash;
>        }
>        if (expectations.contains(Expectation.PASS) && expectations.length ==
1)
> {
>          ++pass;
>        }
>        if (expectations.containsAll([Expectation.FAIL, Expectation.OK]) &&
> -          expectations.length == 2) {
> +      expectations.length == 2) {
>          ++failOk;
>        }
>        if (containsFail && expectations.length == 1) {
>          ++fail;
>        }
>        if (expectations.contains(Expectation.CRASH) &&
> -          expectations.length == 1) {
> +      expectations.length == 1) {
>          ++crash;
>        }
>        if (expectations.contains(Expectation.TIMEOUT)) 
> 
> --- snip-end ---

Check.  Line continuations are at the top of my list to fix.  Definitely before
landing.

FYI unit tests live here: dart/pkg/analyzer/test/services/formatter_test.dart

More integration-like tests to come shortly.

> 
> I'd prefer if we don't ship dartfmt before we have a 
> - an official unambiguous style guide (we do have [0], but I don't know if
that
> is "official")
> - a test suite covering all rules in the style guide
> - dartfmt is passing that test suite (> 95% of the test cases).

Agreed.

> 
> Once we have this achieved, we can run the formatter on all dart files in the
> dart repository, see if we are satisfied with the result, if not, change the
> rules in the style guide and iterate.

+1.  That's the plan.

> 
> We can't use dartfmt to format our testing scripts for example, because the
> indentation is different from how we format our code (as you can see above) [I
> assume that's the same for dart2js ...].
> If we can't use it for our code, our users probably shouldn't either?

We're on the same page.

> 
> [0] https://www.dartlang.org/articles/style-guide/
> 
> Take it as constructive feedback -- the decision when we ship dartfmt as part
of
> the SDK is not mine.

Much appreciated!  As I noted at the outset the build pieces are a parallel
effort to the formatter implementation.  Handling shortcomings like the one you
describe are on the punch list before we consider landing it.  FWIW: I'm only
pushing so hard to have the build bits sorted so that we can have them done and
can solely focus on functionality.

Thanks for your help on both fronts!

> 
> https://codereview.chromium.org/63363007/diff/110001/utils/dartfmt/dartfmt.gyp
> File utils/dartfmt/dartfmt.gyp (right):
> 
>
https://codereview.chromium.org/63363007/diff/110001/utils/dartfmt/dartfmt.gy...
> utils/dartfmt/dartfmt.gyp:29: '../../pkg/analyzer/bin/formatter.dart',
> This command is building a snapshot of .../formatter.dart. A snapshot of a
dart
> application contains *all* dart code (which includes imported packages).
> 
> Therefore you need to add the correct package root here (so the VM can find
the
> imported packages when creating the snapshot). Something like this:
>           'action': [
>             '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)dart<(EXECUTABLE_SUFFIX)',
>             '--snapshot=<(SHARED_INTERMEDIATE_DIR)/dartfmt.dart.snapshot',
>             '--package-root=<(PRODUCT_DIR)/packages/',
>             '../../pkg/analyzer/bin/formatter.dart',
>           ],

Perfect.  Done.  And thanks!

[kustermann, 2013-12-10 07:38:28]

Now we're on the same page. Thanks for the clarification phil.

Please be aware that I'm doing a push-to-trunk every week. This means whatever
we put in the SDK will be shipped to our users within 1 week (so you cannot
commit this CL as it is right now, otherwise our users will have
dart-sdk/bin/dartfmt next Monday).

Let's do it like this:
- You add the dartfmt shell script to sdk/bin/dartfmt and it should work without
building the snapshots or the sdk. (Take a look at sdk/bin/pub how the
--package-root is setup)
- Send out a mail to the dart team, so our developers can test it.
- Once it's "ready", we enable snapshot generation + include it in the sdk.

https://codereview.chromium.org/63363007/diff/130001/sdk/bin/dartfmt
File sdk/bin/dartfmt (right):

https://codereview.chromium.org/63363007/diff/130001/sdk/bin/dartfmt#newcode26
sdk/bin/dartfmt:26: PKG_DIR="$(cd "${SDK_DIR}/../packages" ; pwd -P)"
How  can you assume that there is a packages directory next to the dart-sdk
directory?
Usually our scripts support being invoked like "sdk/bin/dart2js foo.dart
--out=bar.js" inside the sdk (without building the snapshot).

[pquitslund, 2013-12-11 03:41:07]

Moved sdk/bin bits to another CL.  This one can go on hold until we're ready to
land it in the SDK build proper.

https://codereview.chromium.org/63363007/diff/130001/sdk/bin/dartfmt
File sdk/bin/dartfmt (right):

https://codereview.chromium.org/63363007/diff/130001/sdk/bin/dartfmt#newcode26
sdk/bin/dartfmt:26: PKG_DIR="$(cd "${SDK_DIR}/../packages" ; pwd -P)"
On 2013/12/10 07:38:28, kustermann wrote:
> How  can you assume that there is a packages directory next to the dart-sdk
> directory?
> Usually our scripts support being invoked like "sdk/bin/dart2js foo.dart
> --out=bar.js" inside the sdk (without building the snapshot). 

Done.

[pquitslund, 2014-02-09 21:23:13]

These should be the last bits to get 'dartfmt' into the SDK.  PTAL and let me
know if otherwise.

Note that this CL is repurposed.  The other pieces mentioned in the
conversations were broken out and independently landed.

Thanks!

[danrubel, 2014-02-09 22:08:33]

LGTM, but would like Rico's and Martin's thoughts

[ricow1, 2014-02-10 07:12:58]

LGTM

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:1: # Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
since we are landing this now -> 2014

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:21: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg/analyzer"])',
we probably depend on this implicitly by the packages.stamp above, but I am fine
with having this an explicit dependency as well

[kustermann, 2014-02-10 08:31:35]

lgtm

https://codereview.chromium.org/63363007/diff/190001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/63363007/diff/190001/tools/create_sdk.py#newc...
tools/create_sdk.py:23: # ........pub.dart.snapshot
Please update this list with the new snapshot (and dart2js.dart.snapshot while
you're on it, since someone forgot that).

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:21: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg/analyzer"])',
On 2014/02/10 07:12:59, ricow1 wrote:
> we probably depend on this implicitly by the packages.stamp above, but I am
fine
> with having this an explicit dependency as well

The packages.stamp file only needs to change if there are new package
directories (or removed directories in pkg).

I'm not sure what happens if you have a directory as input to an action (which
is what pkg_packages does): does it consider all files (recursively) as an input
or not?

I think we *should* have this here!

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:30: '../../pkg/analyzer/bin/formatter.dart',
I'm sure I raised this question before:
Why can't we just piggy-back on utils_wrapper.dart.snapshot instead of creating
a new snapshot?

I think this will add another 1+ MB. [Are we concerned about start-up time?]

[pquitslund, 2014-02-10 18:51:11]

Thanks for all the help!

I've deferred some of the proposed tweaks to the build wizards.  Feel free to
push ahead as you see fit in a follow-up CL.

Cheers!

https://codereview.chromium.org/63363007/diff/190001/tools/create_sdk.py
File tools/create_sdk.py (right):

https://codereview.chromium.org/63363007/diff/190001/tools/create_sdk.py#newc...
tools/create_sdk.py:23: # ........pub.dart.snapshot
On 2014/02/10 08:31:35, kustermann wrote:
> Please update this list with the new snapshot (and dart2js.dart.snapshot while
> you're on it, since someone forgot that).

Fixed.  Updated bin/ listing too.

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gyp
File utils/dartfmt/dartfmt.gyp (right):

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:1: # Copyright (c) 2013, the Dart project authors. 
Please see the AUTHORS file
On 2014/02/10 07:12:59, ricow1 wrote:
> since we are landing this now -> 2014

Done.

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:21: '<!@(["python", "../../tools/list_files.py",
"\\.dart$", "../../pkg/analyzer"])',
On 2014/02/10 08:31:35, kustermann wrote:
> On 2014/02/10 07:12:59, ricow1 wrote:
> > we probably depend on this implicitly by the packages.stamp above, but I am
> fine
> > with having this an explicit dependency as well
> 
> The packages.stamp file only needs to change if there are new package
> directories (or removed directories in pkg).
> 
> I'm not sure what happens if you have a directory as input to an action (which
> is what pkg_packages does): does it consider all files (recursively) as an
input
> or not?
> 
> I think we *should* have this here!
> 

I have to defer to you guys for this bit (and the next).  We're now officially
out of my depth with the build. :)

https://codereview.chromium.org/63363007/diff/190001/utils/dartfmt/dartfmt.gy...
utils/dartfmt/dartfmt.gyp:30: '../../pkg/analyzer/bin/formatter.dart',
On 2014/02/10 08:31:35, kustermann wrote:
> I'm sure I raised this question before:
> Why can't we just piggy-back on utils_wrapper.dart.snapshot instead of
creating
> a new snapshot?
> 
> I think this will add another 1+ MB. [Are we concerned about start-up time?]
> 

I'm not sure if startup is a huge issue.  Certainly not yet.  Feel free to
change this as you see fit.

[pquitslund, 2014-02-10 19:03:15]

Committed patchset #8 manually as r32512 (presubmit successful).