Global compile option

Global compile option

This enables us to compile, e.g., all angular samples in one shot
(i.e., as one mega module).  This is intended for testing /
transitioning, so perhaps the name should better reflect that.

[vsm, 2016-04-18 18:26:38]

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml#newcode30
pubspec.yaml:30: yaml: ^2.1.2
Not sure I'm abusing convention here, but trying to distinguish what is required
for the lib as opposed to the bin.  It'd be nice to keep the former small.

[Harry Terkelsen, 2016-04-18 18:30:44]

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

[Harry Terkelsen, 2016-04-18 18:30:44]

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml#newcode30
pubspec.yaml:30: yaml: ^2.1.2
On 2016/04/18 18:26:38, vsm wrote:
> Not sure I'm abusing convention here, but trying to distinguish what is
required
> for the lib as opposed to the bin.  It'd be nice to keep the former small.

I think if you do this pub will break you in the future:
https://github.com/dart-lang/pub/issues/1134

[vsm, 2016-04-18 19:51:23]

On 2016/04/18 18:30:44, Harry Terkelsen wrote:
> https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml
> File pubspec.yaml (right):
> 
> https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml#newcode30
> pubspec.yaml:30: yaml: ^2.1.2
> On 2016/04/18 18:26:38, vsm wrote:
> > Not sure I'm abusing convention here, but trying to distinguish what is
> required
> > for the lib as opposed to the bin.  It'd be nice to keep the former small.
> 
> I think if you do this pub will break you in the future:
> https://github.com/dart-lang/pub/issues/1134

Thanks for the heads up.  Thought this might be dodgy.

[vsm, 2016-04-18 19:51:43]

Description was changed from

==========
Global compile option

This enables us to compile, e.g., all angular samples in one shot
(i.e., as one mega module).  This is intended for testing /
transitioning, so perhaps the name should better reflect that.
==========

to

==========
Global compile option

This enables us to compile, e.g., all angular samples in one shot
(i.e., as one mega module).  This is intended for testing /
transitioning, so perhaps the name should better reflect that.
==========

[vsm, 2016-04-18 19:51:43]

vsm@google.com changed reviewers:
+ jmesserly@google.com

[Jennifer Messerly, 2016-04-18 21:45:46]

A few thoughts. Also this could use a test or someone (probably me) will almost
certainly break it. See worker_test for inspiration.

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
File bin/commands/global_compile.dart (right):

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:22: class GlobalCommand extends Command {
WholeProgramCompileCommand? or GlobalCompileCommand

("global" is a bit ambiguous)

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:23: get name => 'global-compile';
in the original design this was called "build". The idea was "compile" is a word
we use for a single unit. "link" combines the units. "build" orchestrates the
process.

In the long run I think a finer grained (per-package at minimum) build+link step
will stress the compiler less. It's not designed to have every file in your
program in memory at once.

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:32: String canonicalize(String uri, String
root) {
is this is the exact code we have in ModuleCompiler? if so should probably move
it into our analyzer helpers file (currently called
compiler/element_helpers.dart, maybe we need a generic analyzer/helpers.dart)
... I'm trying to collect Analyzer workarounds to make it easier to see them and
upstream at some point.

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:35: sourceUri = path.toUri(path.isAbsolute(uri)
? path.absolute(uri) : path.join(root, uri));
looks like this needs dartfmt

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:51: var library =
context.computeLibraryElement(source);
I'm curious, why use resolution?
You can figure out URIs much faster via import parsing:
https://github.com/dart-lang/dev_compiler/blob/39d038366f9080bf00ab242202a19d...

as it is, this will trip on Analyzer's worst case AST caching/eviction behavior
(slower perf & more bugs)

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:75: new ModuleCompiler.withContext(context),
IMO, it would be better to have it use the command line interface. That way it
won't rely on implementation details and is more like what a build tool would
do.

Unlike a watcher/server, this is really just a single compile, so it won't
really benefit from keeping the compiler warm anyway.

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:109: file.writeAsStringSync(module.code, mode:
FileMode.APPEND);
I don't think you want .APPEND, won't this break after the first build?

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:151: String _computeRuntimeDir() {
Vader "NOOOOOOOooooooooooo" meme ... ;)

We just got rid of this after so long. This code is totally broken.

Here are some options:

* Since this is just for Angular test logic, I'm not sure why we need fancy
auto-SDK finding. Make it the script's job. Surely it's easy in the test script
to copy dart_sdk.js from packages/dev_compiler/runtime...?

* Or, move the auto-finding logic to another repository with a "global build
tool" thingy. As long as no one complains when it breaks, because it will break
(it relies on all kinds of implementation details of Pub/snapshots/DDC.)

* It would fine, and probably a lot easier, to embed the JS string so the
command knows the String to output. We had discussed that a while back? It would
be very easy to implement, just tweak ./tool/build_sdk.sh (or the .dart,
whatever makes sense) to wrap dart_sdk.js in a .dart file.

https://codereview.chromium.org/1900813002/diff/1/bin/dartdevc.dart
File bin/dartdevc.dart (right):

https://codereview.chromium.org/1900813002/diff/1/bin/dartdevc.dart#newcode41
bin/dartdevc.dart:41: import 'commands/global_compile.dart';
if we're going to keep this command or something like it,
"package:dev_compiler/src/builder/" was where I was planning to put it.

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/1900813002/diff/1/pubspec.yaml#newcode30
pubspec.yaml:30: yaml: ^2.1.2
On 2016/04/18 18:30:44, Harry Terkelsen wrote:
> On 2016/04/18 18:26:38, vsm wrote:
> > Not sure I'm abusing convention here, but trying to distinguish what is
> required
> > for the lib as opposed to the bin.  It'd be nice to keep the former small.
> 
> I think if you do this pub will break you in the future:
> https://github.com/dart-lang/pub/issues/1134

yeah +1, without bin_dependencies we're kind of stuck here.
that said I don't think you'll need this

[Jennifer Messerly, 2016-04-18 21:49:30]

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
File bin/commands/global_compile.dart (right):

https://codereview.chromium.org/1900813002/diff/1/bin/commands/global_compile...
bin/commands/global_compile.dart:75: new ModuleCompiler.withContext(context),
On 2016/04/18 21:45:45, John Messerly wrote:
> IMO, it would be better to have it use the command line interface. That way it
> won't rely on implementation details and is more like what a build tool would
> do.
> 
> Unlike a watcher/server, this is really just a single compile, so it won't
> really benefit from keeping the compiler warm anyway.

addendum -- a lot depends on where we are going with this.

For a quick hack to get Angular tests working, I don't think we need to check it
in to dev_compiler repo at all. It'll be much better to keep over there. So
that's one option.

If this is a longer term "build" command UI we want to support, then it makes
sense to have it deeply integrated, and try and be smarter about the compilation
units it handles (e.g. per-package).

If we'd rather avoid the long term questions, IMHO it would be better to keep it
localized in the tests, not add it as a fully public command interface. If it's
fully public we need to think about these hard UI questions now.