Add a "global activate" command to pub.

Add a "global activate" command to pub.

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

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

[nweiz, 2014-06-26 22:43:26]

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:15: String get
description => "Make a package globally available.";
"a package's executables"

We're going to have to be very careful to make it clear to users that this
doesn't make packages importable globally, since that's the default mode of
operation of most other package managers. It might be a good idea to mention
this when users run "pub help global" as well.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:26: /// directory is not
inside another entrypoint package.
Make it clear that availability is just in terms of the package's executables.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:49: /// Finds the latest
version of hosted package with [name] that matches
"of hosted" -> "of the hosted"

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:53: var versionFilePath =
path.join(_directory, name + ".version");
It seems a little weird to store the version in a separate file when the
lockfile already exists. Can we just add additional metadata to these lockfiles?

Relatedly, it would probably be a good idea to include information about the
package's source in here as well. I know right now only the hosted source works,
but we don't want to have a backwards-compatibility issue with determining the
source in the future.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:94:
lockFile.serialize(cache.rootDir, cache.sources));
Indent this line.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:99: log.message("Activated
${log.bold(package.name)} ${package.version}.");
For consistency with the message above, maybe word this "$name activated at
$version" (or change the previous message to make it consistent with this).

Also, package.version should be bolded.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:100: // TODO(rnystrom): Look
in "bin" display list of binaries that
"display" -> "and display"

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:107: /// If [version] is not
`null`, will try to maintain that version if possible.
"will" -> "this will"

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:109: VersionConstraint
constraint) {
Nit: this isn't how we usually indent long version lists.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:120: // TODO(rnystrom): Show
most recent unmatching version?
I like this idea.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:121: dataError("Package
$package has no versions that match $constraint.");
Bold the package name. Maybe the constraint too?

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:828: throw new DataException(message);
Is this really worth having a custom function for? It's not used very often and
it doesn't do any extra processing.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/cached_package_test.dart:12:
integration('can active an already cached package', () {
"active" -> "activate"

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/different_version_test.dart:12:
integration("discards the previous active version if it does't match the "
"does't" -> "doesn't"

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/different_version_test.dart:19: //
Activate 1.2.3.
"1.2.3" -> "1.0.0"

[Bob Nystrom, 2014-06-27 00:30:23]

Thanks!

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:15: String get
description => "Make a package globally available.";
On 2014/06/26 22:43:25, nweiz wrote:
> "a package's executables"
> 
> We're going to have to be very careful to make it clear to users that this
> doesn't make packages importable globally, since that's the default mode of
> operation of most other package managers. It might be a good idea to mention
> this when users run "pub help global" as well.

Yeah, reworded.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:26: /// directory is not
inside another entrypoint package.
On 2014/06/26 22:43:25, nweiz wrote:
> Make it clear that availability is just in terms of the package's executables.

Done.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:49: /// Finds the latest
version of hosted package with [name] that matches
On 2014/06/26 22:43:25, nweiz wrote:
> "of hosted" -> "of the hosted"

Done.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:53: var versionFilePath =
path.join(_directory, name + ".version");
On 2014/06/26 22:43:25, nweiz wrote:
> It seems a little weird to store the version in a separate file when the
> lockfile already exists. Can we just add additional metadata to these
lockfiles?

That felt a little weird at first, but I realized I can just do that for the
global lock files and not other ones. Done.

> Relatedly, it would probably be a good idea to include information about the
> package's source in here as well. I know right now only the hosted source
works,
> but we don't want to have a backwards-compatibility issue with determining the
> source in the future.

I realized this too. I have an patch for deactivate I'll send out after this
lands that does that. (But just putting this in the lock file accomplishes the
same thing now.)

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:94:
lockFile.serialize(cache.rootDir, cache.sources));
On 2014/06/26 22:43:25, nweiz wrote:
> Indent this line.

Done.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:99: log.message("Activated
${log.bold(package.name)} ${package.version}.");
On 2014/06/26 22:43:25, nweiz wrote:
> For consistency with the message above, maybe word this "$name activated at
> $version" (or change the previous message to make it consistent with this).

I don't like to start a message with a package name because it looks weird to
start lowercase. I could do "Package foo is now activated at version 1.2.3" here
but I prefer active voice.

> Also, package.version should be bolded.

In other places we only bold the name.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:100: // TODO(rnystrom): Look
in "bin" display list of binaries that
On 2014/06/26 22:43:26, nweiz wrote:
> "display" -> "and display"

Done.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:107: /// If [version] is not
`null`, will try to maintain that version if possible.
On 2014/06/26 22:43:25, nweiz wrote:
> "will" -> "this will"

Done.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:109: VersionConstraint
constraint) {
On 2014/06/26 22:43:25, nweiz wrote:
> Nit: this isn't how we usually indent long version lists.

Done. Just switched to IntelliJ so I'm still getting it set up to format the
right way.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:120: // TODO(rnystrom): Show
most recent unmatching version?
On 2014/06/26 22:43:25, nweiz wrote:
> I like this idea.

Leaving it as a TODO for now, but I do too.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:121: dataError("Package
$package has no versions that match $constraint.");
On 2014/06/26 22:43:25, nweiz wrote:
> Bold the package name. Maybe the constraint too?

Done the former, not the latter.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:828: throw new DataException(message);
On 2014/06/26 22:43:26, nweiz wrote:
> Is this really worth having a custom function for? It's not used very often
and
> it doesn't do any extra processing.

I like having it mainly because it mirrors usageError(), which is used pretty
frequently. However, usageError() has to be on Command (since it accesses the
help stuff from there) and this doesn't. They're conceptually siblings even
though they're defined in different places.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/cached_package_test.dart:12:
integration('can active an already cached package', () {
On 2014/06/26 22:43:26, nweiz wrote:
> "active" -> "activate"

Done.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/different_version_test.dart:12:
integration("discards the previous active version if it does't match the "
On 2014/06/26 22:43:26, nweiz wrote:
> "does't" -> "doesn't"

Do'ne.

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/different_version_test.dart:19: //
Activate 1.2.3.
On 2014/06/26 22:43:26, nweiz wrote:
> "1.2.3" -> "1.0.0"

Done.

[Sean Eagan, 2014-06-30 15:28:51]

On 2014/06/26 21:34:23, Bob Nystrom wrote:

It looks like the requested version constraint is not stored anywhere.  This
means when I later want to upgrade to a new version of a package's executables,
I need to re-specify the version constraint.  Maybe the requested constraints
could be stored in some sort of global pubspec.yaml.

In general, a lot of the actions you might want to do globally seem to map to
package-local actions.  If that's true then it would be nice to not have to
memorize two sets of command names:

    pub get          foo >1.0.0 (see http://www.dartbug.com/18911)
    pub get --global foo >1.0.0 (instead of 'activate')

    pub remove          foo (remove dependency from local pubspec.yaml)
    pub remove --global foo (instead of 'deactivate') (or just require manual
removal from global pubspec.yaml)

    pub upgrade --global
    pub upgrade
    pub upgrade --global foo (instead of re-running `pub global activate foo
<constraint>`)
    pub upgrade          foo (upgrades a single local dependency)

I'm sure that doesn't all completely hang together, but wanted to spill my
thoughts in case it helps.

[nweiz, 2014-06-30 19:42:28]

lgtm

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:99: log.message("Activated
${log.bold(package.name)} ${package.version}.");
On 2014/06/27 00:30:23, Bob Nystrom wrote:
> On 2014/06/26 22:43:25, nweiz wrote:
> > For consistency with the message above, maybe word this "$name activated at
> > $version" (or change the previous message to make it consistent with this).
> 
> I don't like to start a message with a package name because it looks weird to
> start lowercase. I could do "Package foo is now activated at version 1.2.3"
here
> but I prefer active voice.
> 
> > Also, package.version should be bolded.
> 
> In other places we only bold the name.

In that case the "already active" message shouldn't have a bold version number.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:828: throw new DataException(message);
On 2014/06/27 00:30:23, Bob Nystrom wrote:
> On 2014/06/26 22:43:26, nweiz wrote:
> > Is this really worth having a custom function for? It's not used very often
> and
> > it doesn't do any extra processing.
> 
> I like having it mainly because it mirrors usageError(), which is used pretty
> frequently. However, usageError() has to be on Command (since it accesses the
> help stuff from there) and this doesn't. They're conceptually siblings even
> though they're defined in different places.

I don't think this parallel will be clear to people reading the code, but keep
it if you want.

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

https://codereview.chromium.org/331593012/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:24: /// These have been
hand-chosen by the user to make their executables in bin/
"them access them" -> "the user access the executables"

[Bob Nystrom, 2014-06-30 20:00:53]

Committed patchset #2 manually as r37832 (presubmit successful).

[Bob Nystrom, 2014-06-30 20:36:01]

On 2014/06/26 21:34:23, Bob Nystrom wrote:

> It looks like the requested version constraint is not stored anywhere.  This
means when I later want to upgrade to a new version of a package's executables,
I need to re-specify the version constraint.  Maybe the requested constraints
could be stored in some sort of global pubspec.yaml.

I think in practice this won't matter. It supports an arbitrary constraint, but
most users will probably just use nothing ("any") or an explicit version. If you
want to upgrade, that implies the constraint probably is changing anyway.

> In general, a lot of the actions you might want to do globally seem to map to
package-local actions.  If that's true then it would be nice to not have to
memorize two sets of command names:

Yeah, they do parallel somewhat. I split it out deliberately, though, because we
want to make sure people understand the distinction. Global packages are very
disjoint from dependencies. It's important to keep them clear because the
version of a global package may be entirely different than the same version of a
package that you depend on.

Cheers!

- bob

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:99: log.message("Activated
${log.bold(package.name)} ${package.version}.");
On 2014/06/30 19:42:28, nweiz wrote:
> On 2014/06/27 00:30:23, Bob Nystrom wrote:
> > On 2014/06/26 22:43:25, nweiz wrote:
> > > For consistency with the message above, maybe word this "$name activated
at
> > > $version" (or change the previous message to make it consistent with
this).
> > 
> > I don't like to start a message with a package name because it looks weird
to
> > start lowercase. I could do "Package foo is now activated at version 1.2.3"
> here
> > but I prefer active voice.
> > 
> > > Also, package.version should be bolded.
> > 
> > In other places we only bold the name.
> 
> In that case the "already active" message shouldn't have a bold version
number.

I made it bold there to stress that the version is the relevant bit of data.

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

https://codereview.chromium.org/331593012/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:828: throw new DataException(message);
On 2014/06/30 19:42:28, nweiz wrote:
> On 2014/06/27 00:30:23, Bob Nystrom wrote:
> > On 2014/06/26 22:43:26, nweiz wrote:
> > > Is this really worth having a custom function for? It's not used very
often
> > and
> > > it doesn't do any extra processing.
> > 
> > I like having it mainly because it mirrors usageError(), which is used
pretty
> > frequently. However, usageError() has to be on Command (since it accesses
the
> > help stuff from there) and this doesn't. They're conceptually siblings even
> > though they're defined in different places.
> 
> I don't think this parallel will be clear to people reading the code, but keep
> it if you want.

In a later patch, usageError() ends up getting moved too, so I think it will
make a bit more sense.