Use the cached pubspec if possible for describing hosted packages.

Use the cached pubspec if possible for describing hosted packages.

BUG=https://code.google.com/p/dart/issues/detail?id=9027

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

[Bob Nystrom, 2013-04-18 22:23:09]

I did a local test of a package that depends on web-ui and has everything
already in the system cache. This patch gets it from:

real  0m3.666s

to:

real  0m1.468s

Which is pretty nice. Of course, the less you have cached, the less this helps,
but this is an easy optimization to start with.

[nweiz, 2013-04-18 22:50:03]

Don't forget to add a test for corrupted-pubspec recovery.

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart
File utils/pub/hosted_source.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:55: return new Pubspec.load(id.name, cacheDir,
systemCache.sources);
We should catch format/IO errors, print a warning, and fall back to requesting
the pubspec from the server if something goes wrong with loading it. We don't
want a corrupted cache to break "pub install".

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:63: return httpClient.read(url).then((yaml) {
Add a TODO to cache these pubspecs as well. We probably don't want to put them
in the actual system cache directories, but we could have a secondary
pubspec-only cache for packages we haven't downloaded.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:61: /// call to ???.
???

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:62: var _requestedPaths;
This should have a type annotation since it doesn't have an initializer.
Although it might be cleaner if you just initialized it to <String>[] and
checked emptiness/cleared rather than checking nullness/setting to null.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:83: /// ensure it's scheduled with other tasks.
It would be cleaner to make this return a Future<List<String>> and avoid this
schedule caveat. It'll work if you just wrap the body in a call to schedule().

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/update/pub_up...
File utils/tests/pub/update/pub_update_test.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/update/pub_up...
utils/tests/pub/update/pub_update_test.dart:113:
isNot(contains("packages/foo/versions/1.2.3.yaml")));
Shouldn't it actually be empty? It seems like you should assert that instead.

[Bob Nystrom, 2013-04-18 23:07:09]

Thanks!

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart
File utils/pub/hosted_source.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:55: return new Pubspec.load(id.name, cacheDir,
systemCache.sources);
On 2013/04/18 22:50:03, nweiz wrote:
> We should catch format/IO errors, print a warning, and fall back to requesting
> the pubspec from the server if something goes wrong with loading it. We don't
> want a corrupted cache to break "pub install".

Done.

https://codereview.chromium.org/14241005/diff/1/utils/pub/hosted_source.dart#...
utils/pub/hosted_source.dart:63: return httpClient.read(url).then((yaml) {
On 2013/04/18 22:50:03, nweiz wrote:
> Add a TODO to cache these pubspecs as well. We probably don't want to put them
> in the actual system cache directories, but we could have a secondary
> pubspec-only cache for packages we haven't downloaded.

Done.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:61: /// call to ???.
On 2013/04/18 22:50:03, nweiz wrote:
> ???

Oops. Wrote that comment before I figured out what to call the method. :) Fixed.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:62: var _requestedPaths;
On 2013/04/18 22:50:03, nweiz wrote:
> This should have a type annotation since it doesn't have an initializer.
> Although it might be cleaner if you just initialized it to <String>[] and
> checked emptiness/cleared rather than checking nullness/setting to null.

Done.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:83: /// ensure it's scheduled with other tasks.
On 2013/04/18 22:50:03, nweiz wrote:
> It would be cleaner to make this return a Future<List<String>> and avoid this
> schedule caveat. It'll work if you just wrap the body in a call to schedule().

Done.

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/update/pub_up...
File utils/tests/pub/update/pub_update_test.dart (right):

https://codereview.chromium.org/14241005/diff/1/utils/tests/pub/update/pub_up...
utils/tests/pub/update/pub_update_test.dart:113:
isNot(contains("packages/foo/versions/1.2.3.yaml")));
On 2013/04/18 22:50:03, nweiz wrote:
> Shouldn't it actually be empty? It seems like you should assert that instead.

It will contain a request for the version list for foo. I don't want to assert
that fact since it isn't really relevant to this test, so I just assert that it
doesn't contain a request for the pubspec.

[nweiz, 2013-04-19 00:12:31]

This still needs a test for catching a pubspec format error.

https://codereview.chromium.org/14241005/diff/3002/utils/pub/hosted_source.dart
File utils/pub/hosted_source.dart (right):

https://codereview.chromium.org/14241005/diff/3002/utils/pub/hosted_source.da...
utils/pub/hosted_source.dart:57: } on FormatException catch(error) {
Nit: space between "catch" and "(error)".

We should really catch IO exceptions here, too, in case the pubspec is
unreadable or something.

Maybe split out an isIoException function from isUserFacingException?

https://codereview.chromium.org/14241005/diff/3002/utils/pub/hosted_source.da...
utils/pub/hosted_source.dart:60: log.fine("Cached pubspec for $id had format
error:\n$error");
Print the stack trace as well.

https://codereview.chromium.org/14241005/diff/3002/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/14241005/diff/3002/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:85: _requestedPaths = <String>[];
I think I prefer [List.clear] here. That also lets you make _requestedPaths
final.

[Bob Nystrom, 2013-04-19 21:39:27]

So it turns out this "look in the cache for an installed pubspec" was already
supported directly by SystemCache and HostedSource didn't need to do it.

The only problem was that (oops) the version solver's cache didn't go through
that, so it requested it directly from the Source. I just fixed that.

I didn't add a test for how it handles a corrupted pubspec. Apparently pub will
already die a horrible death in that case independent of these changes. We
should handle that better, but I think that's out of scope of this patch.

Sorry for the code churn here, but please PTAL.

https://codereview.chromium.org/14241005/diff/3002/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/14241005/diff/3002/utils/tests/pub/test_pub.d...
utils/tests/pub/test_pub.dart:85: _requestedPaths = <String>[];
On 2013/04/19 00:12:31, nweiz wrote:
> I think I prefer [List.clear] here. That also lets you make _requestedPaths
> final.

Done.

[nweiz, 2013-04-19 22:13:49]

lgtm

Can you file a bug for pub crashing horribly on a corrupted pubspec?

[Bob Nystrom, 2013-04-19 22:21:41]

On 2013/04/19 22:13:49, nweiz wrote:
> lgtm
> 
> Can you file a bug for pub crashing horribly on a corrupted pubspec?

Done! https://code.google.com/p/dart/issues/detail?id=10065

I was a bit surprised by this too, but I guess we've never actually had the
situation arise.

[Bob Nystrom, 2013-04-19 22:29:49]

Committed patchset #3 manually as r21777 (presubmit successful).