Add offline support to pub install and update.

Add offline support to pub install and update.

If you pass "--offline" to either, it will treat your system cache
as if it were the package server. Running update in offline also
prints a warning that you may have selected an older version of a
package.

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

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

[Bob Nystrom, 2013-05-02 20:12:55]

Dan, this supersedes your patch so you can probably delete that one. It works
the same way as yours modulo tests, my suggestions on that patch, and the cached
packages stuff Keerti added.

[nweiz, 2013-05-06 23:02:25]

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command.dart:72: /// Override this to use
offline-only sources instead of hitting the network.
It's a little confusing that this only has an effect if it's based on
[commandOptions], not anything that happens after [cache] is initialized.
Document that.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command_install.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command_install.dart:23: help: 'Use cached
packages instead of accessing the network');
Trailing period.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command_update.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command_update.dart:25: help: 'Use cached packages
instead of accessing the network');
Trailing period.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/hosted_source.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:101: var url =
_getSourceDirectory(parsed.last);
This variable should probably be "dir" or something now that it's not actually a
URL.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:165: /// This is the modified
hosted source used when pub install or updated are run
"updated" -> "update"

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:168: class OfflineHostedSource
extends HostedSource {
I don't like making "--offline" hosted-specific. It's reasonable to want to
install from an offline git repo as well; the same goes for any hypothetical
source that uses the network.

What do you think about adding an "isOffline" flag to the base Source class
instead?

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:195: // getVersions() will only
return packages that are already cached.
"getVersions()" -> "[getVersions]", same for "describe()" below

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:196: // SystemCache should only
call describe() on a package after its failed to
"its" -> "it's" or "it has"

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/system_cache.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/system_cache.dart:52: if (isOffline == null)
isOffline = false;
I think I prefer explicitly defaulting named boolean parameters, although I
don't feel too strongly about it.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/test/up...
File sdk/lib/_internal/pub/test/update/hosted/offline_test.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/test/up...
sdk/lib/_internal/pub/test/update/hosted/offline_test.dart:37: });
We should test failure cases here, too.

[Bob Nystrom, 2013-05-07 21:03:09]

Thanks!

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command.dart:72: /// Override this to use
offline-only sources instead of hitting the network.
On 2013/05/06 23:02:25, nweiz wrote:
> It's a little confusing that this only has an effect if it's based on
> [commandOptions], not anything that happens after [cache] is initialized.
> Document that.

Done.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command_install.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command_install.dart:23: help: 'Use cached
packages instead of accessing the network');
On 2013/05/06 23:02:25, nweiz wrote:
> Trailing period.

We use periods for the command descriptions but not for the options. (I'm not
sure why we do that, but this is at least consistent with the other commands.)

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command_update.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command_update.dart:25: help: 'Use cached packages
instead of accessing the network');
On 2013/05/06 23:02:25, nweiz wrote:
> Trailing period.

Ditto.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/hosted_source.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:101: var url =
_getSourceDirectory(parsed.last);
On 2013/05/06 23:02:25, nweiz wrote:
> This variable should probably be "dir" or something now that it's not actually
a
> URL.

Done.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/system_cache.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/system_cache.dart:52: if (isOffline == null)
isOffline = false;
On 2013/05/06 23:02:25, nweiz wrote:
> I think I prefer explicitly defaulting named boolean parameters, although I
> don't feel too strongly about it.

I don't like the syntax for named parameter defaults but done. For methods that
can be overridden (unlike this one), I do prefer to avoid defaults though,
because it avoids needing to duplicate it in the subclass.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/test/up...
File sdk/lib/_internal/pub/test/update/hosted/offline_test.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/test/up...
sdk/lib/_internal/pub/test/update/hosted/offline_test.dart:37: });
On 2013/05/06 23:02:25, nweiz wrote:
> We should test failure cases here, too.

Done.

[nweiz, 2013-05-07 21:14:49]

You didn't respond to any of my comments about "--offline" being
hosted-specific.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command_install.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command_install.dart:23: help: 'Use cached
packages instead of accessing the network');
On 2013/05/07 21:03:09, Bob Nystrom wrote:
> On 2013/05/06 23:02:25, nweiz wrote:
> > Trailing period.
> 
> We use periods for the command descriptions but not for the options. (I'm not
> sure why we do that, but this is at least consistent with the other commands.)

All the global options have periods. I think it's just the publish/uploader
options that don't; those should be fixed.

[Bob Nystrom, 2013-05-07 22:20:33]

Oops, sorry. Was trying to use the keyboard shortcuts in Reitveld and
accidentally skipped some stuff.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/hosted_source.dart (right):

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:165: /// This is the modified
hosted source used when pub install or updated are run
On 2013/05/06 23:02:25, nweiz wrote:
> "updated" -> "update"

Done.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:168: class OfflineHostedSource
extends HostedSource {
On 2013/05/06 23:02:25, nweiz wrote:
> I don't like making "--offline" hosted-specific. It's reasonable to want to
> install from an offline git repo as well; the same goes for any hypothetical
> source that uses the network.

I considered that too, but didn't want to implement that right now. This is
slightly time-sensitive because we'd like this to be in for Google IO.

> 
> What do you think about adding an "isOffline" flag to the base Source class
> instead?

Dan's patch worked that way. The end result was that most of the methods in
HostedSource were just a big if statement. I figured that's what dynamic
dispatch is for, so in this patch I split out the source.

I'd definitely be up for having an OfflineGitSource that subclasses GitSource
like this one. Then SystemCache can switch that in too.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:195: // getVersions() will only
return packages that are already cached.
On 2013/05/06 23:02:25, nweiz wrote:
> "getVersions()" -> "[getVersions]", same for "describe()" below

Done.

https://codereview.chromium.org/14680005/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/hosted_source.dart:196: // SystemCache should only
call describe() on a package after its failed to
On 2013/05/06 23:02:25, nweiz wrote:
> "its" -> "it's" or "it has"

Done.

[nweiz, 2013-05-07 23:54:15]

lgtm

[Bob Nystrom, 2013-05-08 00:40:02]

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