Make binstubs run snapshots directly when possible.

Make binstubs run snapshots directly when possible.

R=nweiz@google.com

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

[Bob Nystrom, 2014-09-19 21:40:33]

rnystrom@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2014-09-22 20:01:42]

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:241: /// Returns
the paths of the snapshots that were successfully precompiled.
It seems weird that the snapshots are only implicitly associated with the ids
that generated them. I think this API would make more sense if it returned a map
from asset ids to snapshot paths. If you don't want to do that, at least
document how the returned paths relate to the executables that were compiled.

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:274: // Don't
return this future, since we have no need to wait for the
"return" -> "await"

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:449: key: (snapshot) =>
p.basenameWithoutExtension(
This should assert that the snapshot comes from the right package.

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:556: invocation = "dart
$snapshot";
If you use a path that's relative to [binStubPath] here, that'll make this work
even if [snapshot] is relative to the working directory. It'll also ensure that
this won't break if the path to the user's pub cache contains spaces or if they
move their pub cache later.

[Bob Nystrom, 2014-09-22 22:55:28]

Thanks!

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:241: /// Returns
the paths of the snapshots that were successfully precompiled.
On 2014/09/22 20:01:42, nweiz wrote:
> It seems weird that the snapshots are only implicitly associated with the ids
> that generated them. I think this API would make more sense if it returned a
map
> from asset ids to snapshot paths. If you don't want to do that, at least
> document how the returned paths relate to the executables that were compiled.

Done.

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:274: // Don't
return this future, since we have no need to wait for the
On 2014/09/22 20:01:42, nweiz wrote:
> "return" -> "await"

Done.

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:449: key: (snapshot) =>
p.basenameWithoutExtension(
On 2014/09/22 20:01:42, nweiz wrote:
> This should assert that the snapshot comes from the right package.

N/A.

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:556: invocation = "dart
$snapshot";
On 2014/09/22 20:01:42, nweiz wrote:
> If you use a path that's relative to [binStubPath] here, that'll make this
work
> even if [snapshot] is relative to the working directory. It'll also ensure
that
> this won't break if the path to the user's pub cache contains spaces or if
they
> move their pub cache later.

If I just make it a relative path, The Dart VM interprets it relative to the
cwd, not the bash script path (naturally), so that doesn't work. I looked into
putting a relative path in here and doing some bash magic to expand it correctly
before handing it to Dart, but all the docs I read made me feel like that was
strictly more hairy than just using an absolute path.

I did quote it to handle spaces.

[nweiz, 2014-09-23 00:15:45]

lgtm

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:556: invocation = "dart
$snapshot";
On 2014/09/22 22:55:28, Bob Nystrom wrote:
> On 2014/09/22 20:01:42, nweiz wrote:
> > If you use a path that's relative to [binStubPath] here, that'll make this
> work
> > even if [snapshot] is relative to the working directory. It'll also ensure
> that
> > this won't break if the path to the user's pub cache contains spaces or if
> they
> > move their pub cache later.
> 
> If I just make it a relative path, The Dart VM interprets it relative to the
> cwd, not the bash script path (naturally), so that doesn't work. I looked into
> putting a relative path in here and doing some bash magic to expand it
correctly
> before handing it to Dart, but all the docs I read made me feel like that was
> strictly more hairy than just using an absolute path.
> 
> I did quote it to handle spaces.

In that case, call [p.absolute] to ensure that the executable still works if
[snapshot] is passed to this method as a relative path.

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

https://codereview.chromium.org/586173002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:241: /// Returns a
map from executable ID to path for the snapshots that were
This says "executable ID" but actually returns the executable name.

[Bob Nystrom, 2014-09-23 18:22:36]

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

[Bob Nystrom, 2014-09-23 18:23:27]

Thanks!

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

https://codereview.chromium.org/586173002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:556: invocation = "dart
$snapshot";
On 2014/09/23 00:15:44, nweiz wrote:
> On 2014/09/22 22:55:28, Bob Nystrom wrote:
> > On 2014/09/22 20:01:42, nweiz wrote:
> > > If you use a path that's relative to [binStubPath] here, that'll make this
> > work
> > > even if [snapshot] is relative to the working directory. It'll also ensure
> > that
> > > this won't break if the path to the user's pub cache contains spaces or if
> > they
> > > move their pub cache later.
> > 
> > If I just make it a relative path, The Dart VM interprets it relative to the
> > cwd, not the bash script path (naturally), so that doesn't work. I looked
into
> > putting a relative path in here and doing some bash magic to expand it
> correctly
> > before handing it to Dart, but all the docs I read made me feel like that
was
> > strictly more hairy than just using an absolute path.
> > 
> > I did quote it to handle spaces.
> 
> In that case, call [p.absolute] to ensure that the executable still works if
> [snapshot] is passed to this method as a relative path.

The paths we get are absolute now, so I just added an assert() that requires
that.

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

https://codereview.chromium.org/586173002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:241: /// Returns a
map from executable ID to path for the snapshots that were
On 2014/09/23 00:15:44, nweiz wrote:
> This says "executable ID" but actually returns the executable name.

Done.