Support directories other than "web" in pub build.

Support directories other than "web" in pub build.

BUG=https://code.google.com/p/dart/issues/detail?id=14673
R=nweiz@google.com

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

[Bob Nystrom, 2014-01-18 00:36:52]

This patch is pretty huge, but it's mostly moving code around and cleaning stuff
up.

This also does *not* support other directories for pub serve. I'll do that in a
separate (much smaller) patch. There's a small bit of temporary code here that
will go away when that's done.

[nweiz, 2014-01-28 03:21:51]

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:1: // Copyright (c)
2013, the Dart project authors.  Please see the AUTHORS file
2014. Same for other new files

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:32: class
BuildEnvironment {
This class confuses me, since the only public stuff it exposes is [server] and
[dart2JSTransformer], and the latter is sort of hacky anyway. Should this just
be exposed as a function that returns a BarbackServer?

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:39: /// Includes
[buildDirectories] in the root package.
" in addition to `lib/` and `asset/`."

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:76: }
It might be cleaner if we do this above and just reassign [useDart2JS] rather
than removing the Dart2JSTransformer after the fact.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:88: .then((server)
=> environment);
"server" => "_"

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:149: if
(!completer.isCompleted) completer.complete(server);
"server" is unused, so this should probably just be [completer.complete].

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:26: ].toSet();
Style nit: I slightly prefer [new Set.from] because it puts the fact that this
is going to be a set right up front.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:44: int builtFiles = 0;
It's weird that this is public but [_buildDirectories] is private.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:75:
environment.dart2JSTransformer.entrypoints);
Accessing dart2JSTransformer here is gross. It also doesn't seem safe to assume
that the browser JS files only need to go next to the ".dart.js" files. What if
an HTML file imports a dart2js-compiled file that's not in the same directory?

If we do make this assumption, it seems cleaner to look for ".dart.js" files and
to copy the JS bootstrap files next to those, or to have the dart2js transformer
emit them itself.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:95: /// exist.
Passing * on the command line is very confusing. It makes users think about the
difference between "pub build *" and "pub build \*", which is asking for
trouble. I think we should either do "pub build all" or "pub build --all",
depending on whether or not we think people will want to build a directory named
"all/" at some point.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:107: throw new
ApplicationException("There are no buildable directories.");
List which directories are considered buildable in this error message.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:132: 'The allowed directories
are: $allowed.');
Nit: since [allowed] is sentenceized, I think this would read better without the
colon.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:142: throw new
ApplicationException('Could not find $dirs $names.');
I feel like "Could not find" is more ambiguous than "$dirs $names do(es) not
exist."

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:173: // If the asset is from a
special "shared" directory package, copy it into
"'shared' directory package" -> "'assets/' directory"

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_benchmark_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_benchmark_test.dart:51:
}
This test is confusing. If you're testing that "benchmark/" is allowed, this
should run "pub build benchmark". There should be a separate test to validate
that "pub build *" picks up all the correct directories.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart:34:
]),
I don't like testing "example" here as well. This test should focus on testing
that assets from dependencies get included; a separate test should test that
asset directories are included in every directory under build.

[Bob Nystrom, 2014-01-28 23:02:03]

Thanks!

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:1: // Copyright (c)
2013, the Dart project authors.  Please see the AUTHORS file
On 2014/01/28 03:21:51, nweiz wrote:
> 2014. Same for other new files

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:32: class
BuildEnvironment {
On 2014/01/28 03:21:51, nweiz wrote:
> This class confuses me, since the only public stuff it exposes is [server] and
> [dart2JSTransformer], and the latter is sort of hacky anyway.

It's exposing more stuff now.

> Should this just be exposed as a function that returns a BarbackServer? 

The previous code was more top-level-function-y and I think it outgrew that
style. We ended up passing an increasing number of arguments around. This
class's public API is pretty lean right now, but it's convenient to be able to
pass it around since it maintains a decent amount of internal state.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:39: /// Includes
[buildDirectories] in the root package.
On 2014/01/28 03:21:51, nweiz wrote:
> " in addition to `lib/` and `asset/`."

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:76: }
On 2014/01/28 03:21:51, nweiz wrote:
> It might be cleaner if we do this above and just reassign [useDart2JS] rather
> than removing the Dart2JSTransformer after the fact.

Done. This brought to my attention that we weren't handling configured dart2js
in pub build, so I fixed that. That involved moving some code around to plumb
BuildEnvironment through, but I think it's cleaner now.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:88: .then((server)
=> environment);
On 2014/01/28 03:21:51, nweiz wrote:
> "server" => "_"

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:149: if
(!completer.isCompleted) completer.complete(server);
On 2014/01/28 03:21:51, nweiz wrote:
> "server" is unused, so this should probably just be [completer.complete].

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:26: ].toSet();
On 2014/01/28 03:21:51, nweiz wrote:
> Style nit: I slightly prefer [new Set.from] because it puts the fact that this
> is going to be a set right up front.

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:44: int builtFiles = 0;
On 2014/01/28 03:21:51, nweiz wrote:
> It's weird that this is public but [_buildDirectories] is private.

Made _buildDirectories public. In the command classes, we tend to be lax about
making stuff private.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:75:
environment.dart2JSTransformer.entrypoints);
On 2014/01/28 03:21:51, nweiz wrote:
> Accessing dart2JSTransformer here is gross. It also doesn't seem safe to
assume
> that the browser JS files only need to go next to the ".dart.js" files. What
if
> an HTML file imports a dart2js-compiled file that's not in the same directory?
> 
> If we do make this assumption, it seems cleaner to look for ".dart.js" files
and
> to copy the JS bootstrap files next to those, or to have the dart2js
transformer
> emit them itself.

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:95: /// exist.
On 2014/01/28 03:21:51, nweiz wrote:
> Passing * on the command line is very confusing. It makes users think about
the
> difference between "pub build *" and "pub build \*", which is asking for
> trouble. I think we should either do "pub build all" or "pub build --all",
> depending on whether or not we think people will want to build a directory
named
> "all/" at some point.

Done. --all.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:107: throw new
ApplicationException("There are no buildable directories.");
On 2014/01/28 03:21:51, nweiz wrote:
> List which directories are considered buildable in this error message.

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:132: 'The allowed directories
are: $allowed.');
On 2014/01/28 03:21:51, nweiz wrote:
> Nit: since [allowed] is sentenceized, I think this would read better without
the
> colon.

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:142: throw new
ApplicationException('Could not find $dirs $names.');
On 2014/01/28 03:21:51, nweiz wrote:
> I feel like "Could not find" is more ambiguous than "$dirs $names do(es) not
> exist."

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/build.dart:173: // If the asset is from a
special "shared" directory package, copy it into
On 2014/01/28 03:21:51, nweiz wrote:
> "'shared' directory package" -> "'assets/' directory"

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_benchmark_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/compiles_entrypoints_in_benchmark_test.dart:51:
}
On 2014/01/28 03:21:51, nweiz wrote:
> This test is confusing. If you're testing that "benchmark/" is allowed, this
> should run "pub build benchmark". There should be a separate test to validate
> that "pub build *" picks up all the correct directories.

Done.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart:34:
]),
On 2014/01/28 03:21:51, nweiz wrote:
> I don't like testing "example" here as well. This test should focus on testing
> that assets from dependencies get included; a separate test should test that
> asset directories are included in every directory under build.

In order to test that they are included, they have to be included *somewhere*,
so this test would be implicitly testing some top level directory. I figured it
was simplest to test more than one of them here instead of having several 90%
identical tests.

[nweiz, 2014-01-29 01:24:34]

A few suggestions, otherwise lgtm. Passing BuildEnvironment around is definitely
cleaner.

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart:34:
]),
On 2014/01/28 23:02:04, Bob Nystrom wrote:
> On 2014/01/28 03:21:51, nweiz wrote:
> > I don't like testing "example" here as well. This test should focus on
testing
> > that assets from dependencies get included; a separate test should test that
> > asset directories are included in every directory under build.
> 
> In order to test that they are included, they have to be included *somewhere*,
> so this test would be implicitly testing some top level directory. I figured
it
> was simplest to test more than one of them here instead of having several 90%
> identical tests.

I'd rather just test (any) one directory and consider the behavior of "does the
same thing to all top-level directories" to be orthogonal to this test.

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:27:
Iterable<Transformer> builtInTransformers) {
Since we're moving towards treating BuildEnvironment as the encapsulation of all
configuration, it feels wrong to pass in [builtInTransformers] alongside it
here. It would feel better either to have it be a getter on BuildEnvironment, or
for [loadAllTransformers] to compute it based on the configuration provided by
BuildEnvironment.

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:130: return 0;
It sort of feels like this should be [exit_codes.SUCCESS].

[Bob Nystrom, 2014-01-29 19:03:37]

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
File
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart
(right):

https://codereview.chromium.org/141113011/diff/1/sdk/lib/_internal/pub/test/b...
sdk/lib/_internal/pub/test/build/includes_assets_from_dependencies_test.dart:34:
]),
On 2014/01/29 01:24:35, nweiz wrote:
> On 2014/01/28 23:02:04, Bob Nystrom wrote:
> > On 2014/01/28 03:21:51, nweiz wrote:
> > > I don't like testing "example" here as well. This test should focus on
> testing
> > > that assets from dependencies get included; a separate test should test
that
> > > asset directories are included in every directory under build.
> > 
> > In order to test that they are included, they have to be included
*somewhere*,
> > so this test would be implicitly testing some top level directory. I figured
> it
> > was simplest to test more than one of them here instead of having several
90%
> > identical tests.
> 
> I'd rather just test (any) one directory and consider the behavior of "does
the
> same thing to all top-level directories" to be orthogonal to this test.

We don't tend to go minimal in our integration tests. For example, even if this
test only tests one top level directory, you'll note its still testing assets
both in the main directory and in subdirectories of asset.

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:27:
Iterable<Transformer> builtInTransformers) {
On 2014/01/29 01:24:35, nweiz wrote:
> Since we're moving towards treating BuildEnvironment as the encapsulation of
all
> configuration, it feels wrong to pass in [builtInTransformers] alongside it
> here. It would feel better either to have it be a getter on BuildEnvironment,
or
> for [loadAllTransformers] to compute it based on the configuration provided by
> BuildEnvironment.

I agree. Done.

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/command/build.dart (right):

https://codereview.chromium.org/141113011/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/build.dart:130: return 0;
On 2014/01/29 01:24:35, nweiz wrote:
> It sort of feels like this should be [exit_codes.SUCCESS].

Done.

[Bob Nystrom, 2014-01-29 19:04:28]

Committed patchset #3 manually as r32131 (presubmit successful).