Add a "cache add" command to download packages to the cache.

Add a "cache add" command to download packages to the cache.

BUG=https://code.google.com/p/dart/issues/detail?id=16265
R=nweiz@google.com

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

[nweiz, 2014-02-07 01:20:41]

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:1: // Copyright (c) 2012,
the Dart project authors.  Please see the AUTHORS file
2014

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:19: String get description
=> "Install a package system-wide.";
I don't like this description. It seems to imply that either this will make the
package's executable available, or that it will make the package available
system-wide without needing individual pubspec declarations; neither of these is
accurate.

How about "Download a package to the system cache"?

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:27: help: "Install all
matching versions.",
As above, I feel like "Download" is a better term than "Install" here.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:38: this.printUsage("");
Why are you passing the empty string here? Don't we want to print the
description here?

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:48: }
Should we add an [unexpectedArguments] utility method?

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:60: }
Why not just let the FormatException be handled by the top-level exception
handler? It's considered a user-facing exception, so it'll be printed nicely.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:63: var source =
this.cache.sources["hosted"];
Add a TODO to enable caching of git sources.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:64: return
source.getVersions(package, package).then((versions) {
Add a TODO to make the server customizable.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:69: "Package $package has
no versions that match $constraint.");
Don't include the "that match" bit if the user didn't pass a constraint.

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/version.dart:45: /// reversed of [compareTo] which
sorts in ascending order.
It seems confusing to me that this is in the (approximately) reverse order of
[compareTo]. Why not make them go in the same direction?

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/adds_latest_matching_version_test.dart
(right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/adds_latest_matching_version_test.dart:24:
d.cacheDir({"foo": "1.2.3"}).validate();
Might be good to validate that 1.2.2 didn't get added here.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/cache/add/all_with_some_versions_present_test.dart
(right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/all_with_some_versions_present_test.dart:33:
Downloading foo 2.0.0...''');
It might look nicer if the output were more aligned, like so:

    foo 1.2.1: already cached.
    foo 1.2.2: downloading...
    foo 1.2.3: already cached.
    foo 2.0.0: downloading...

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/missing_package_arg_test.dart (right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/missing_package_arg_test.dart:15: exitCode:
exit_codes.USAGE);
Validate that this also prints usage information.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/no_matching_version_test.dart (right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/no_matching_version_test.dart:18: error:
'Package foo has no versions that match >2.0.0.',
Maybe print the most recent version or something?

[Bob Nystrom, 2014-02-07 17:22:44]

Thanks!

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:1: // Copyright (c) 2012,
the Dart project authors.  Please see the AUTHORS file
On 2014/02/07 01:20:41, nweiz wrote:
> 2014

Done.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:19: String get description
=> "Install a package system-wide.";
On 2014/02/07 01:20:41, nweiz wrote:
> I don't like this description. It seems to imply that either this will make
the
> package's executable available, or that it will make the package available
> system-wide without needing individual pubspec declarations; neither of these
is
> accurate.
> 
> How about "Download a package to the system cache"?

We talked about later adding support for caching stuff from git repos, so
"download" feels to specific. How about just "Install a package." since the
parent command already mentions the system cache?

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:27: help: "Install all
matching versions.",
On 2014/02/07 01:20:41, nweiz wrote:
> As above, I feel like "Download" is a better term than "Install" here.

See above.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:38: this.printUsage("");
On 2014/02/07 01:20:41, nweiz wrote:
> Why are you passing the empty string here? Don't we want to print the
> description here?

Changed to usageError().

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:48: }
On 2014/02/07 01:20:41, nweiz wrote:
> Should we add an [unexpectedArguments] utility method?

Maybe eventually, but for now I just changed this to usageError().

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:60: }
On 2014/02/07 01:20:41, nweiz wrote:
> Why not just let the FormatException be handled by the top-level exception
> handler? It's considered a user-facing exception, so it'll be printed nicely.

It wouldn't have the right exit code. Changed this to a usageError() too, which
will show the usage info.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:63: var source =
this.cache.sources["hosted"];
On 2014/02/07 01:20:41, nweiz wrote:
> Add a TODO to enable caching of git sources.

Done.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:64: return
source.getVersions(package, package).then((versions) {
On 2014/02/07 01:20:41, nweiz wrote:
> Add a TODO to make the server customizable.

Done.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:69: "Package $package has
no versions that match $constraint.");
On 2014/02/07 01:20:41, nweiz wrote:
> Don't include the "that match" bit if the user didn't pass a constraint.

Is that even possible in practice? If the package doesn't exist at all, we won't
get here, and there's no way to have a package on the server that has no
versions.

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/version.dart:45: /// reversed of [compareTo] which
sorts in ascending order.
On 2014/02/07 01:20:41, nweiz wrote:
> It seems confusing to me that this is in the (approximately) reverse order of
> [compareTo]. Why not make them go in the same direction?

It is a bit confusing, which is why the docs call it out, but this is the order
that's actually most useful. Since the verb is "priority", I think the *first*
item in the result should be the one that gets first priority, which means
descending order.

The behavior here is, I think, the most natural one, it's just different from
sorting.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/adds_latest_matching_version_test.dart
(right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/adds_latest_matching_version_test.dart:24:
d.cacheDir({"foo": "1.2.3"}).validate();
On 2014/02/07 01:20:41, nweiz wrote:
> Might be good to validate that 1.2.2 didn't get added here.

Done.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File
sdk/lib/_internal/pub/test/cache/add/all_with_some_versions_present_test.dart
(right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/all_with_some_versions_present_test.dart:33:
Downloading foo 2.0.0...''');
On 2014/02/07 01:20:41, nweiz wrote:
> It might look nicer if the output were more aligned, like so:
> 
>     foo 1.2.1: already cached.
>     foo 1.2.2: downloading...
>     foo 1.2.3: already cached.
>     foo 2.0.0: downloading...

Yeah, I thought about that, but leading with a lowercase package name bugged me.
Also, the "Downloading foo..." message is the same output used during pub get,
so I didn't want to mess with it.

This isn't a command we expect people to use frequently, so I didn't put too
much time into its output.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/missing_package_arg_test.dart (right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/missing_package_arg_test.dart:15: exitCode:
exit_codes.USAGE);
On 2014/02/07 01:20:41, nweiz wrote:
> Validate that this also prints usage information.

Done.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/cache/add/no_matching_version_test.dart (right):

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/cache/add/no_matching_version_test.dart:18: error:
'Package foo has no versions that match >2.0.0.',
On 2014/02/07 01:20:41, nweiz wrote:
> Maybe print the most recent version or something?

Added a TODO.

[Bob Nystrom, 2014-02-10 16:49:59]

Friendly ping!

- bob

[nweiz, 2014-02-10 22:39:30]

A couple more comments, otherwise lgtm.

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:19: String get description
=> "Install a package system-wide.";
On 2014/02/07 17:22:44, Bob Nystrom wrote:
> On 2014/02/07 01:20:41, nweiz wrote:
> > I don't like this description. It seems to imply that either this will make
> the
> > package's executable available, or that it will make the package available
> > system-wide without needing individual pubspec declarations; neither of
these
> is
> > accurate.
> > 
> > How about "Download a package to the system cache"?
> 
> We talked about later adding support for caching stuff from git repos, so
> "download" feels to specific. How about just "Install a package." since the
> parent command already mentions the system cache?

"Install" definitely feels like the operation that will make binaries available.
I think it makes sense to say that you download a git repo.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:60: }
On 2014/02/07 17:22:44, Bob Nystrom wrote:
> On 2014/02/07 01:20:41, nweiz wrote:
> > Why not just let the FormatException be handled by the top-level exception
> > handler? It's considered a user-facing exception, so it'll be printed
nicely.
> 
> It wouldn't have the right exit code. Changed this to a usageError() too,
which
> will show the usage info.

I think a format usage error is fine for a mis-formatted version, but sure.

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:69: "Package $package has
no versions that match $constraint.");
On 2014/02/07 17:22:44, Bob Nystrom wrote:
> On 2014/02/07 01:20:41, nweiz wrote:
> > Don't include the "that match" bit if the user didn't pass a constraint.
> 
> Is that even possible in practice? If the package doesn't exist at all, we
won't
> get here, and there's no way to have a package on the server that has no
> versions.

Good point.

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/version.dart:45: /// reversed of [compareTo] which
sorts in ascending order.
On 2014/02/07 17:22:44, Bob Nystrom wrote:
> On 2014/02/07 01:20:41, nweiz wrote:
> > It seems confusing to me that this is in the (approximately) reverse order
of
> > [compareTo]. Why not make them go in the same direction?
> 
> It is a bit confusing, which is why the docs call it out, but this is the
order
> that's actually most useful. Since the verb is "priority", I think the *first*
> item in the result should be the one that gets first priority, which means
> descending order.
> 
> The behavior here is, I think, the most natural one, it's just different from
> sorting.

When I say "sort by priority", I think I mean ascending, just like when I say
"sort alphabetically" or "sort by size" or any other sort.

[Bob Nystrom, 2014-02-11 19:42:09]

Thanks!

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/cache_add.dart:60: }
On 2014/02/10 22:39:30, nweiz wrote:
> On 2014/02/07 17:22:44, Bob Nystrom wrote:
> > On 2014/02/07 01:20:41, nweiz wrote:
> > > Why not just let the FormatException be handled by the top-level exception
> > > handler? It's considered a user-facing exception, so it'll be printed
> nicely.
> > 
> > It wouldn't have the right exit code. Changed this to a usageError() too,
> which
> > will show the usage info.
> 
> I think a format usage error is fine for a mis-formatted version, but sure.

Right, usage error is the right exit code. But an uncaught FormatException exits
with a data exit code.

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

https://codereview.chromium.org/136063005/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/version.dart:45: /// reversed of [compareTo] which
sorts in ascending order.
On 2014/02/10 22:39:30, nweiz wrote:
> On 2014/02/07 17:22:44, Bob Nystrom wrote:
> > On 2014/02/07 01:20:41, nweiz wrote:
> > > It seems confusing to me that this is in the (approximately) reverse order
> of
> > > [compareTo]. Why not make them go in the same direction?
> > 
> > It is a bit confusing, which is why the docs call it out, but this is the
> order
> > that's actually most useful. Since the verb is "priority", I think the
*first*
> > item in the result should be the one that gets first priority, which means
> > descending order.
> > 
> > The behavior here is, I think, the most natural one, it's just different
from
> > sorting.
> 
> When I say "sort by priority", I think I mean ascending, just like when I say
> "sort alphabetically" or "sort by size" or any other sort.

Done.

[Bob Nystrom, 2014-02-11 19:42:41]

Committed patchset #4 manually as r32575 (presubmit successful).