Precompile immutable dependencies on "pub get".

Precompile immutable dependencies on "pub get".

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

[Bob Nystrom, 2014-09-18 19:32:01]

I'm super excited about this.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:98: var package
= environment.graph.packages[packageName];
Is there a reason for this change?

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:157: /// Precompile any
transformed dependencies of the entrypoint.
Can we use a different term here? I associate "precompile" with snapshotting
executables. Maybe "transformDependencies"?

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:176: return overlaps(
Document this.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:186: await
log.progress("Precompiling dependencies", () async {
How about "Transforming dependencies"?

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:215:
toSentence(ordered(dependenciesToPrecompile).map(log.bold)) + ".");
This will often be a long enough list that I think it's worth writing each on
its own line.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package_graph.dart:87: if (depId == null) return
true;
This is for handling a dependency back on the root, right? Document that.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart
(right):

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart:214:
integration("caches the dependency in debug mode", () {
I think this is redundant with the earlier tests.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart:240: //
TODO(nweiz): Implement this.
Are you doing this in a later patch?

[nweiz, 2014-09-18 21:20:39]

Code review changes

[nweiz, 2014-09-18 21:22:33]

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:98: var package
= environment.graph.packages[packageName];
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> Is there a reason for this change?

Yeah. [environment.packages] is the restricted set of packages that only
includes those necessary for doing the specific compilation we need right now.
Other packages aren't even loaded in the asset environment. If we try to load
transformers for them, everything goes pear-shaped.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:157: /// Precompile any
transformed dependencies of the entrypoint.
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> Can we use a different term here? I associate "precompile" with snapshotting
> executables. Maybe "transformDependencies"?

I don't like that the verb "transform" doesn't make any distinction between the
sort of transformation that's done eagerly for caching and the sort that's done
on-demand. "Precompile" does make that distinction, and I think it's a more
important one than the form of the cached information. For example, if the VM
added support for snapshotting libraries, we'd use it here; it wouldn't change
the semantics of the method.

If you really want a distinction between the two, I'd rather keep this as
[precompileDependencies] and change [precompileExecutables] to
[snapshotExecutables]. But that wouldn't match the text we surface to the users,
so I'm not a huge fan.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:176: return overlaps(
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:186: await
log.progress("Precompiling dependencies", () async {
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> How about "Transforming dependencies"?

I don't think the end user cares about the underlying mechanics we use to
precompile stuff. As such, this should have the same text as the snapshotting
process, and I think "Precompiling" is clearer than "Transforming" for that
purpose, since the "pre" implies that it's being done for caching purposes.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:215:
toSentence(ordered(dependenciesToPrecompile).map(log.bold)) + ".");
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> This will often be a long enough list that I think it's worth writing each on
> its own line.

I'm not sure that's true. For a polymer app there are, I think, three
dependencies that get precompiled. I'd like to add some line-wrapping logic to
pub at some point, but I think for now this will read better on one line on
average than split across many.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package_graph.dart:87: if (depId == null) return
true;
On 2014/09/18 19:32:00, Bob Nystrom wrote:
> This is for handling a dependency back on the root, right? Document that.

Done.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart
(right):

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart:214:
integration("caches the dependency in debug mode", () {
On 2014/09/18 19:32:01, Bob Nystrom wrote:
> I think this is redundant with the earlier tests.

How so? The earlier tests don't ensure that the actual mode flag is set
correctly.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart:240: //
TODO(nweiz): Implement this.
On 2014/09/18 19:32:01, Bob Nystrom wrote:
> Are you doing this in a later patch?

Yep.

[Bob Nystrom, 2014-09-18 22:03:02]

I'd really like a more meaningful term than "precompiling". Otherwise, LGTM.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:98: var package
= environment.graph.packages[packageName];
On 2014/09/18 21:22:33, nweiz wrote:
> On 2014/09/18 19:32:00, Bob Nystrom wrote:
> > Is there a reason for this change?
> 
> Yeah. [environment.packages] is the restricted set of packages that only
> includes those necessary for doing the specific compilation we need right now.
> Other packages aren't even loaded in the asset environment. If we try to load
> transformers for them, everything goes pear-shaped.

Ah, I missed the ".graph" that was removed. Now I get it.

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

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:186: await
log.progress("Precompiling dependencies", () async {
On 2014/09/18 21:22:33, nweiz wrote:
> On 2014/09/18 19:32:00, Bob Nystrom wrote:
> > How about "Transforming dependencies"?
> 
> I don't think the end user cares about the underlying mechanics we use to
> precompile stuff. As such, this should have the same text as the snapshotting
> process, and I think "Precompiling" is clearer than "Transforming" for that
> purpose, since the "pre" implies that it's being done for caching purposes.

We're giving them *some* verb and they have to understand that it does
something. "Transform" is a verb that we've very clearly taught them to be aware
of, and does describe what's going on here. "Precompile" is a novel term. If we
feel it's useful enough to tell the user that something is going on here, we may
as well indicate what's happening, which is that transformers are running.

Maybe "Pretransforming"? I'd still prefer "transforming" because the "pre" part
is obvious—the user knows they're running pub get right now.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:215:
toSentence(ordered(dependenciesToPrecompile).map(log.bold)) + ".");
On 2014/09/18 21:22:33, nweiz wrote:
> On 2014/09/18 19:32:00, Bob Nystrom wrote:
> > This will often be a long enough list that I think it's worth writing each
on
> > its own line.
> 
> I'm not sure that's true. For a polymer app there are, I think, three
> dependencies that get precompiled. I'd like to add some line-wrapping logic to
> pub at some point, but I think for now this will read better on one line on
> average than split across many.

OK.

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart
(right):

https://codereview.chromium.org/576333002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/get/cache_transformed_dependency_test.dart:214:
integration("caches the dependency in debug mode", () {
On 2014/09/18 21:22:33, nweiz wrote:
> On 2014/09/18 19:32:01, Bob Nystrom wrote:
> > I think this is redundant with the earlier tests.
> 
> How so? The earlier tests don't ensure that the actual mode flag is set
> correctly.

Oh, now I see. I just meant that they do test that the cached stuff is in
"/debug/...".

[nweiz, 2014-09-18 23:04:44]

Committed patchset #2 (id:20001) manually as 40477 (presubmit successful).