Precompile immutable globally-installed pub executables.

Precompile immutable globally-installed pub executables.

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

[Bob Nystrom, 2014-08-25 23:13:24]

You should also test that deactivating a package removes its precompiled
executables.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:236: ///
Precompiles all [packageName]'s executables to snapshots in [directory].
"all" -> "all of".

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:243: return
graph.entrypoint.loadPackageGraph()
Isn't graph.entrypoint.loadPackage() redundant?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:259:
log.message("Precompiled ${_executableName(id)}.");
Nit, but bold the executable?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:283: String
_executableName(AssetId id) =>
I didn't realize in the above call that this would be bold. How about
_formatExecutable()?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:15: import 'executable.dart'
as exe;
I don't think a prefix is needed here.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:134: return new
Entrypoint.inMemory(root, lockFile, cache).
Put the "." on the next line.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:142: Future
_precompileExecutables(Entrypoint entrypoint, String package) {
This doesn't check the case where you activate a git package that contains a
path dependency.

I'm fine with just adding a TODO for now but we should figure out what the plan
is.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:180: // lockfiles anymore.
Add a tracking bug.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:292: /// recompiled if
necessary and then run. Otherwise, it will be run from
This reads weird. If it's precompiled, why would it have to be recompiled?
Explain that it's checking the SDK.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:312: if
(readTextFile(sdkVersionPath) == "${sdk.version}\n") return null;
Log at fine level what's going on here.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:318: var vmArgs =
[p.join(binDir, '$executable.dart.snapshot')]..addAll(args);
This seems like the kind of logic that also belongs in executable.dart. At
least, that's what I had in mind. How do you feel about moving it there?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:335: p.join(_directory, name,
"pubspec.lock");
Did you move this for any particular reason?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:178: return !file.contains('/',
relativeStart);
=>

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/global/activate/activate_git_after_hosted_test.dart
(right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/global/activate/activate_git_after_hosted_test.dart:37:
Precompiled foo:foo.""");
What happened to the activated message?

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_git_test.dart
(right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_git_test.dart:40:
Precompiled foo:foo.""");
I think these should print before the activated message.

[nweiz, 2014-08-26 21:04:17]

Code review changes

[nweiz, 2014-08-26 21:04:22]

> Precompile immutable globally-installed pub executables.

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:236: ///
Precompiles all [packageName]'s executables to snapshots in [directory].
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> "all" -> "all of".

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:243: return
graph.entrypoint.loadPackageGraph()
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> Isn't graph.entrypoint.loadPackage() redundant?

...yes.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:259:
log.message("Precompiled ${_executableName(id)}.");
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> Nit, but bold the executable?

_executableName does this itself.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:283: String
_executableName(AssetId id) =>
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> I didn't realize in the above call that this would be bold. How about
> _formatExecutable()?

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:15: import 'executable.dart'
as exe;
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> I don't think a prefix is needed here.

It is because we want to call runExecutable from within
GlobalPackages.runExecutable.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:134: return new
Entrypoint.inMemory(root, lockFile, cache).
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> Put the "." on the next line.

Oops, too Ruby.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:142: Future
_precompileExecutables(Entrypoint entrypoint, String package) {
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> This doesn't check the case where you activate a git package that contains a
> path dependency.
> 
> I'm fine with just adding a TODO for now but we should figure out what the
plan
> is.

That's a weird use-case, to say the least, but TODO added.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:180: // lockfiles anymore.
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> Add a tracking bug.

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:292: /// recompiled if
necessary and then run. Otherwise, it will be run from
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> This reads weird. If it's precompiled, why would it have to be recompiled?
> Explain that it's checking the SDK.

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:312: if
(readTextFile(sdkVersionPath) == "${sdk.version}\n") return null;
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> Log at fine level what's going on here.

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:335: p.join(_directory, name,
"pubspec.lock");
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> Did you move this for any particular reason?

Now that each package has its own directory in the global cache, it seemed very
weird for the lockfile not to be in that directory.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:178: return !file.contains('/',
relativeStart);
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> =>

Done.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/global/activate/activate_git_after_hosted_test.dart
(right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/global/activate/activate_git_after_hosted_test.dart:37:
Precompiled foo:foo.""");
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> What happened to the activated message?

It still exists and this test was busted :p.

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_git_test.dart
(right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_git_test.dart:40:
Precompiled foo:foo.""");
On 2014/08/25 23:13:24, Bob Nystrom wrote:
> I think these should print before the activated message.

Done.

[Bob Nystrom, 2014-09-02 20:29:30]

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:318: var vmArgs =
[p.join(binDir, '$executable.dart.snapshot')]..addAll(args);
On 2014/08/25 23:13:23, Bob Nystrom wrote:
> This seems like the kind of logic that also belongs in executable.dart. At
> least, that's what I had in mind. How do you feel about moving it there?

I think you missed responding to this comment.

[nweiz, 2014-09-02 23:41:50]

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/global_packages.dart (right):

https://codereview.chromium.org/475093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:318: var vmArgs =
[p.join(binDir, '$executable.dart.snapshot')]..addAll(args);
On 2014/09/02 20:29:30, Bob Nystrom wrote:
> On 2014/08/25 23:13:23, Bob Nystrom wrote:
> > This seems like the kind of logic that also belongs in executable.dart. At
> > least, that's what I had in mind. How do you feel about moving it there?
> 
> I think you missed responding to this comment.

I forgot to reply but I did move the logic out.

[Bob Nystrom, 2014-09-02 23:58:54]

lgtm

[nweiz, 2014-09-03 00:21:03]

Committed patchset #3 (id:40001) manually as 39787 (presubmit successful).