Support configuring dart2js from the pubspec.

Support configuring dart2js from the pubspec.

R=rnystrom@google.com
BUG=14664
Committed: https://code.google.com/p/dart/source/detail?r=31755

[Bob Nystrom, 2014-01-10 22:23:04]

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:99: throw new
FormatException('Unsupported built-in transformer $package.');
This will make forward compatibility harder. If someone has an older version of
pub and tries to use a package that uses a new built-in transformer, this will
fail.

We could possibly report a warning and then just skip any unknown built-in
transformer names?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:148: var explicitDart2Js =
graph.entrypoint.root.pubspec.transformers.any(
How about "containsDart2js" or something else boolean sounding?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:149: (transformers) =>
transformers.any((id) => id.package == '\$dart2js'));
One limitation is that users can't put dart2js in a transformer group. Do we
care about that?

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: 'analyzeAll',
'suppressWarnings', 'suppressHints', 'terse'
Sort?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:61:
"${toSentence(invalidOptions.map((option) => '"$option"'))}.");
I think for backwards compat, we'll want to just warn and ignore these.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:153: 'of
strings).');
Do we want to allow a single string too?

$dart2js:
  commandLineOptions: "--blah --foo blorg"

Seems reasonable for users to want to try.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:173: /// [def] is
the default value of the option.
How about "defaultsTo" to be consistent with the args package? When I read the
code above, I guessed "def" meant "define" or something else.

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:63: bool toDart: false}) {
This is quite a pile of parameters. The only code calling this is
Dart2JSTransformer. Maybe we should just move the compile code completely into
there?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:77: options.add('-D$name=$value');
Do we need to worry about escaping these?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_option_test.dart
(right):

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_option_test.dart:32:
requestShould404("main.dart.js");
Can we validate more directly that it logs an error?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/output_can_be_consumed_by_successive_phases.dart
(right):

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/output_can_be_consumed_by_successive_phases.dart:36:
integration("minify configuration overrides the mode", () {
Update description.

[nweiz, 2014-01-11 00:54:36]

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:99: throw new
FormatException('Unsupported built-in transformer $package.');
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> This will make forward compatibility harder. If someone has an older version
of
> pub and tries to use a package that uses a new built-in transformer, this will
> fail.
> 
> We could possibly report a warning and then just skip any unknown built-in
> transformer names?

I don't think skipping transformers is safe here. In this hypothetical future,
the user has declared the transformer because they want it to do something... if
it just does nothing instead, the chance that their app works correctly is quite
low.

It's worth noting that due to the lazy parsing of pubspecs, this error will only
be raised when doing something that actually works with transformers -- either
"pub serve" or "pub build". It won't break their ability to run "pub get" etc.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:148: var explicitDart2Js =
graph.entrypoint.root.pubspec.transformers.any(
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> How about "containsDart2js" or something else boolean sounding?

Done.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback.dart:149: (transformers) =>
transformers.any((id) => id.package == '\$dart2js'));
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> One limitation is that users can't put dart2js in a transformer group. Do we
> care about that?

Not for now.

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: 'analyzeAll',
'suppressWarnings', 'suppressHints', 'terse'
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> Sort?

In all cases they're ordered the same as "dart2js --help", which I think will
make it easier to modify them later. If you disagree I can re-order them.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:61:
"${toSentence(invalidOptions.map((option) => '"$option"'))}.");
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> I think for backwards compat, we'll want to just warn and ignore these.

Same issue as earlier: options control behavior, and I don't think we can be
that blasé about assuming that an unknown option can be ignored without
consequence.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:153: 'of
strings).');
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> Do we want to allow a single string too?
> 
> $dart2js:
>   commandLineOptions: "--blah --foo blorg"
> 
> Seems reasonable for users to want to try.

I thought about that, but I feel like the difference between shell parsing and
just `split(' ')` would be weird. If we had a shell parser library, I'd be all
for it, but we don't, so it makes me pretty nervous.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:173: /// [def] is
the default value of the option.
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> How about "defaultsTo" to be consistent with the args package? When I read the
> code above, I guessed "def" meant "define" or something else.

Done.

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

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:63: bool toDart: false}) {
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> This is quite a pile of parameters. The only code calling this is
> Dart2JSTransformer. Maybe we should just move the compile code completely into
> there?

I like having a library function for this. Who knows when we'll need to invoke
dart2js elsewhere?

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:77: options.add('-D$name=$value');
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> Do we need to worry about escaping these?

I don't think so... and I'm not sure how we'd do so anyway. "A list of strings
representing shell arguments" isn't exactly a well-defined transfer format :p.

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_option_test.dart
(right):

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/does_not_support_invalid_command_line_option_test.dart:32:
requestShould404("main.dart.js");
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> Can we validate more directly that it logs an error?

Done. Good thing you suggested this, it turns out a bunch of these were 404ing
for the wrong reason. Oops!

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/output_can_be_consumed_by_successive_phases.dart
(right):

https://codereview.chromium.org/133843006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/output_can_be_consumed_by_successive_phases.dart:36:
integration("minify configuration overrides the mode", () {
On 2014/01/10 22:23:05, Bob Nystrom wrote:
> Update description.

Done.

[Bob Nystrom, 2014-01-11 01:25:21]

I'm still worried we're going to regret being so narrow about validating
options, but LGTM.

[nweiz, 2014-01-13 20:48:04]

Committed patchset #2 manually as r31755 (presubmit successful).

[blois, 2014-01-22 00:05:40]

https://codereview.chromium.org/133843006/diff/90001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/133843006/diff/90001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/dart.dart:87: options.add('--asdfsadfsadf');
What does this option do?

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

https://codereview.chromium.org/133843006/diff/90001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/133843006/diff/90001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/dart.dart:87: options.add('--asdfsadfsadf');
On 2014/01/22 00:05:41, blois wrote:
> What does this option do?

Leftover from debugging.