Make pub's binstubs resilient to changes in snapshot format.

Make pub's binstubs resilient to changes in snapshot format.

This also makes "pub cache repair" repair activated packages, including updating
the binstub format.

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

[nweiz, 2014-11-21 01:09:56]

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:441: log.error(
I'm not sure whether this should delete the binstub. On the one hand, the
existing "cache repair" stuff doesn't and it seems scary to just delete stuff,
but on the other hand not doing so risks leaving the cache in a broken
state—running the script will continue to be busted. What do you think?

[Bob Nystrom, 2014-11-21 19:13:41]

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:57: final bool _inMemory;
Can we determine this from the root package? Will its [dir] be null for
in-memory packages?

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:377: /// Shows to the user
formatted list of globally activated packages.
While you're here, this thing needs some grammar work.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:441: log.error(
On 2014/11/21 01:09:55, nweiz wrote:
> I'm not sure whether this should delete the binstub. On the one hand, the
> existing "cache repair" stuff doesn't and it seems scary to just delete stuff,
> but on the other hand not doing so risks leaving the cache in a broken
> state—running the script will continue to be busted. What do you think?

I like the idea of deleting it. It doesn't contain any unique data that can't be
reconstituted, and it avoids leaving something known bad on disk.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:452: await
Future.wait(listDir(_directory).map((entry) async {
I think you should use a loop here. Otherwise, it's possible to interleave
output between "Reactivating..." and a later error.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:485: "packages:\n");
What are these from? Should they be deleted?

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:520: bool pathSuggestion:
true}) {
"suggestion" is a weird name for a boolean to me. How about
"suggestIfNotOnPath"?

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:661: if errorlevel 255 (
Document this.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:664: 
Delete the snapshot here.

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:191: if (error is Error && trace != null)
trace = error.stackTrace;
==

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/c...
File sdk/lib/_internal/pub/test/cache/repair/updates_binstubs_test.dart (right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/updates_binstubs_test.dart:60:
d.matcherFile(binStubName('foo-script'), contains('255'))
Document why you're looking for 255.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/activate/outdated_binstub_test.dart
(right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/outdated_binstub_test.dart:53:
d.matcherFile(binStubName('foo-script'), contains("Binstub: 2"))
What is "Binstub: 2"?

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/binstubs/outdated_snapshot_test.dart
(right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/binstubs/outdated_snapshot_test.dart:47: //
process.stdout.expect(consumeThrough("ok [arg1, arg2]"));
Delete?

[nweiz, 2014-11-22 02:29:57]

Code review changes

[nweiz, 2014-11-22 02:32:56]

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:57: final bool _inMemory;
On 2014/11/21 19:13:40, Bob Nystrom wrote:
> Can we determine this from the root package? Will its [dir] be null for
> in-memory packages?

Good idea. Done.

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:377: /// Shows to the user
formatted list of globally activated packages.
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> While you're here, this thing needs some grammar work.

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:441: log.error(
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> On 2014/11/21 01:09:55, nweiz wrote:
> > I'm not sure whether this should delete the binstub. On the one hand, the
> > existing "cache repair" stuff doesn't and it seems scary to just delete
stuff,
> > but on the other hand not doing so risks leaving the cache in a broken
> > state—running the script will continue to be busted. What do you think?
> 
> I like the idea of deleting it. It doesn't contain any unique data that can't
be
> reconstituted, and it avoids leaving something known bad on disk.

Done. Should we plan to do the same for cached packages?

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:452: await
Future.wait(listDir(_directory).map((entry) async {
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> I think you should use a loop here. Otherwise, it's possible to interleave
> output between "Reactivating..." and a later error.

That's contrary to how repairing cached packages works. I'm not opposed to
changing it, but it should be consistent either way.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:485: "packages:\n");
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> What are these from? Should they be deleted?

They could be from any number of things, but most likely is probably a
deactivate gone wrong. If we're deleting corrupted binstubs, we should delete
these as well.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:520: bool pathSuggestion:
true}) {
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> "suggestion" is a weird name for a boolean to me. How about
> "suggestIfNotOnPath"?

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:661: if errorlevel 255 (
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> Document this.

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:664: 
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> Delete the snapshot here.

Actually, deleting the snapshot is wrong; it prevents "pub global" from
rebuilding the snapshot. Removed from the Bash version.

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:191: if (error is Error && trace != null)
trace = error.stackTrace;
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> ==

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/c...
File sdk/lib/_internal/pub/test/cache/repair/updates_binstubs_test.dart (right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/c...
sdk/lib/_internal/pub/test/cache/repair/updates_binstubs_test.dart:60:
d.matcherFile(binStubName('foo-script'), contains('255'))
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> Document why you're looking for 255.

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/activate/outdated_binstub_test.dart
(right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/outdated_binstub_test.dart:53:
d.matcherFile(binStubName('foo-script'), contains("Binstub: 2"))
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> What is "Binstub: 2"?

An outdated verification that's not actually running because I call [create]
instead of [validate] :p. Fixed.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/binstubs/outdated_snapshot_test.dart
(right):

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/binstubs/outdated_snapshot_test.dart:47: //
process.stdout.expect(consumeThrough("ok [arg1, arg2]"));
On 2014/11/21 19:13:41, Bob Nystrom wrote:
> Delete?

Actually, uncomment.

[Bob Nystrom, 2014-12-01 18:29:25]

Couple of suggestions but LGTM!

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:441: log.error(
On 2014/11/22 02:32:55, nweiz wrote:
> On 2014/11/21 19:13:41, Bob Nystrom wrote:
> > On 2014/11/21 01:09:55, nweiz wrote:
> > > I'm not sure whether this should delete the binstub. On the one hand, the
> > > existing "cache repair" stuff doesn't and it seems scary to just delete
> stuff,
> > > but on the other hand not doing so risks leaving the cache in a broken
> > > state—running the script will continue to be busted. What do you think?
> > 
> > I like the idea of deleting it. It doesn't contain any unique data that
can't
> be
> > reconstituted, and it avoids leaving something known bad on disk.
> 
> Done. Should we plan to do the same for cached packages?

Yeah, I think so. We probably want to handle the deletion failing too.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:452: await
Future.wait(listDir(_directory).map((entry) async {
On 2014/11/22 02:32:56, nweiz wrote:
> On 2014/11/21 19:13:41, Bob Nystrom wrote:
> > I think you should use a loop here. Otherwise, it's possible to interleave
> > output between "Reactivating..." and a later error.
> 
> That's contrary to how repairing cached packages works. I'm not opposed to
> changing it, but it should be consistent either way.

Does repairing have any other output aside from the initial log of the package
name? Either way, I agree on consistency. I'd be fine if repairing used a loop
too.

[nweiz, 2014-12-01 23:43:28]

Code review changes

[nweiz, 2014-12-01 23:44:39]

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

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:441: log.error(
On 2014/12/01 18:29:24, Bob Nystrom wrote:
> On 2014/11/22 02:32:55, nweiz wrote:
> > On 2014/11/21 19:13:41, Bob Nystrom wrote:
> > > On 2014/11/21 01:09:55, nweiz wrote:
> > > > I'm not sure whether this should delete the binstub. On the one hand,
the
> > > > existing "cache repair" stuff doesn't and it seems scary to just delete
> > stuff,
> > > > but on the other hand not doing so risks leaving the cache in a broken
> > > > state—running the script will continue to be busted. What do you think?
> > > 
> > > I like the idea of deleting it. It doesn't contain any unique data that
> can't
> > be
> > > reconstituted, and it avoids leaving something known bad on disk.
> > 
> > Done. Should we plan to do the same for cached packages?
> 
> Yeah, I think so. We probably want to handle the deletion failing too.

Done.

https://codereview.chromium.org/745153002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:452: await
Future.wait(listDir(_directory).map((entry) async {
On 2014/12/01 18:29:24, Bob Nystrom wrote:
> On 2014/11/22 02:32:56, nweiz wrote:
> > On 2014/11/21 19:13:41, Bob Nystrom wrote:
> > > I think you should use a loop here. Otherwise, it's possible to interleave
> > > output between "Reactivating..." and a later error.
> > 
> > That's  to how repairing cached packages works. I'm not opposed to
> > changing it, but it should be consistent either way.
> 
> Does repairing have any other output aside from the initial log of the package
> name?

Repairing could also have an error.

> Either way, I agree on consistency. I'd be fine if repairing used a loop too.

Done.

[nweiz, 2014-12-01 23:45:19]

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