Support activating packages from local paths.

Support activating packages from Git and local paths.

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

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

R=nweiz@google.com

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

[nweiz, 2014-07-31 20:01:54]

There should be tests for activating a package from git before activating it
from another source, after activating it from another source, before activating
it from a different git URL, and before activating it from a different ref of
the same URL.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:16: String get usage
=> "pub global activate [--source source] <package> [version]";
Long line. Also, normally we don't include options in the usage string.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:49: // TODO(bob):
Test.
Remove this

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:50: var ref =
readArg("Must provide a Git ref for the package.");
Why does the user have to supply a ref for git? Git has a notion of a default
ref; we should use it.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:59: constraint = new
VersionConstraint.parse(readArg(""));
Passing an empty string to parseArg here reads weird. I'd make the argument
optional.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:69: id =
lockFile.packages[name];
I don't understand this logic. If there's already a lockfile for this package,
shouldn't we override it with the (potentially) new ref? If we're not overriding
it, why are we running _installInCache?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:110: _describeActive(name);
Add a comment about why this is is being called despite its return value going
unused.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:120: /// Installs the package
[id] with [lockFile] into the system cache.
Clarify that this also installs [id]'s dependencies.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:165: void
_finishActivation(PackageId id, LockFile lockFile) {
It would be nice to have a more descriptive method name here. Maybe
"_writeLockFile"?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:180: /// Gets the lock file
for the currently activate package with [name].
"activate" -> "active"

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:194: } else {
Shouldn't a git source display its URL and ref?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:196: "version
${log.bold(id.version)}.");
"is already active" sounds to me like the sort of thing you'd see in an error
message describing why activation failed. Maybe "was active" instead to make it
clear that it's being replaced with the new source/version?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:201: // If we couldn't read
the lock file, it's not activated.
Return null here or the analyzer will complain.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/path.dart:29: static String
pathFromDescription(description) => description["path"];
If this path is relative, won't it be relative to the wrong directory?

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_local_test.dart:31:
Package foo is already active at path "${canonicalize(p.join(sandboxDir,
"foo"))}".
Long line

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/activate_path_after_hosted_test.dart:29:
Activated foo 2.0.0.""");
It's weird that this doesn't indicate that it's switching sources. Maybe the
"already active" should indicate that it's from a hosted source if it's
switching to a non-hosted source?

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/does_not_install_dependencies_for_path_test.dart:23:
output: "Activated foo 1.0.0.");
This doesn't seem like the behavior users would want. It seems like, if the
dependencies haven't yet been installed for a path package, they should be when
that package is activated.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/git_package_test.dart:20:
schedulePub(args: ["global", "activate", "-sgit", "foo", "../foo.git"],
Now I'm really confused. Where's the ref? What's this "foo" doing here?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/deactivate/remote_package_test.dart
(right):

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/deactivate/remote_package_test.dart:1: //
Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
Nit: it seems like this should be called hosted_package_test.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
File
sdk/lib/_internal/pub/test/global/run/reflects_changes_to_local_package_test.dart
(right):

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/run/reflects_changes_to_local_package_test.dart:10:
integration('runs a script in a local activated package', () {
This should match the test file name.

[Bob Nystrom, 2014-07-31 21:00:43]

Committed patchset #1 manually as r38798 (presubmit successful).

[Bob Nystrom, 2014-07-31 22:26:27]

On 2014/07/31 21:00:43, Bob Nystrom wrote:
> Committed patchset #1 manually as r38798 (presubmit successful).

Sorry, I didn't mean to commit this. Reverted here:
https://codereview.chromium.org/438773002/

I'll respond to the code review here and then start a new review after that.

- bob

[Bob Nystrom, 2014-07-31 22:27:26]

On 2014/07/31 22:26:27, Bob Nystrom wrote:
> I'll respond to the code review here and then start a new review after that.

Scratch that. I think I can just re-open this review, so I'll keep it here.

- bob

[Bob Nystrom, 2014-08-04 23:41:52]

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:16: String get usage
=> "pub global activate [--source source] <package> [version]";
On 2014/07/31 20:01:53, nweiz wrote:
> Long line. Also, normally we don't include options in the usage string.

Tweaked it a bit.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:49: // TODO(bob):
Test.
On 2014/07/31 20:01:53, nweiz wrote:
> Remove this

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:50: var ref =
readArg("Must provide a Git ref for the package.");
On 2014/07/31 20:01:53, nweiz wrote:
> Why does the user have to supply a ref for git? Git has a notion of a default
> ref; we should use it.

I think I just got my terminology confused here. This was really just the repo.

Killing git support for now.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:59: constraint = new
VersionConstraint.parse(readArg(""));
On 2014/07/31 20:01:53, nweiz wrote:
> Passing an empty string to parseArg here reads weird. I'd make the argument
> optional.

Done.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:69: id =
lockFile.packages[name];
On 2014/07/31 20:01:53, nweiz wrote:
> I don't understand this logic. If there's already a lockfile for this package,
> shouldn't we override it with the (potentially) new ref? If we're not
overriding
> it, why are we running _installInCache?

Killing git support for now.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:110: _describeActive(name);
On 2014/07/31 20:01:53, nweiz wrote:
> Add a comment about why this is is being called despite its return value going
> unused.

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:120: /// Installs the package
[id] with [lockFile] into the system cache.
On 2014/07/31 20:01:54, nweiz wrote:
> Clarify that this also installs [id]'s dependencies.

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:165: void
_finishActivation(PackageId id, LockFile lockFile) {
On 2014/07/31 20:01:53, nweiz wrote:
> It would be nice to have a more descriptive method name here. Maybe
> "_writeLockFile"?

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:180: /// Gets the lock file
for the currently activate package with [name].
On 2014/07/31 20:01:53, nweiz wrote:
> "activate" -> "active"

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:194: } else {
On 2014/07/31 20:01:54, nweiz wrote:
> Shouldn't a git source display its URL and ref?

Killing git support for now.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:196: "version
${log.bold(id.version)}.");
On 2014/07/31 20:01:53, nweiz wrote:
> "is already active" sounds to me like the sort of thing you'd see in an error
> message describing why activation failed. Maybe "was active" instead to make
it
> clear that it's being replaced with the new source/version?

Putting it in the past tense may be confusing if activating the new package
fails. Changed to "is currently active". Is that better?

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:201: // If we couldn't read
the lock file, it's not activated.
On 2014/07/31 20:01:53, nweiz wrote:
> Return null here or the analyzer will complain.

Done.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/source/path.dart:29: static String
pathFromDescription(description) => description["path"];
On 2014/07/31 20:01:54, nweiz wrote:
> If this path is relative, won't it be relative to the wrong directory?

GlobalPackages ensures that the path is already made absolute before it gets
here, but I clarified the behavior of this method.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/activate_hosted_after_local_test.dart:31:
Package foo is already active at path "${canonicalize(p.join(sandboxDir,
"foo"))}".
On 2014/07/31 20:01:54, nweiz wrote:
> Long line

Done.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/activate_path_after_hosted_test.dart:29:
Activated foo 2.0.0.""");
On 2014/07/31 20:01:54, nweiz wrote:
> It's weird that this doesn't indicate that it's switching sources. Maybe the
> "already active" should indicate that it's from a hosted source if it's
> switching to a non-hosted source?

Done. Active hosted packages just show the version number. Path ones will show
the path.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/does_not_install_dependencies_for_path_test.dart:23:
output: "Activated foo 1.0.0.");
On 2014/07/31 20:01:54, nweiz wrote:
> This doesn't seem like the behavior users would want. It seems like, if the
> dependencies haven't yet been installed for a path package, they should be
when
> that package is activated.

When you activate a package at a path, it relies on the lock file in that
directory. I felt like implicitly running pub get in *that* directory when you
want pub global activate wasn't the behavior users would expect, but if we don't
do that, we can't install the dependencies.

In practice, I don't think it matters much: most users activating a package at a
path will already have a lockfile which implies they'll already have the
dependencies. If they don't, pub global run *will* implicitly run pub get.

I think it's useful for pub global activate to eagerly pull down dependencies
when you activate remote (hosted/git) packages because you may be about to go
offline. But for path packages, you probably have the deps already and/or know
what you're doing, so it's less important to do the get eagerly.

You're right that this is a semantically strange corner case. I added this test
specifically to track the current behavior since I didn't realize it at first
either. If you think it makes sense to implicitly run pub get on the activated
directory, I could be persuaded.

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/git_package_test.dart:20:
schedulePub(args: ["global", "activate", "-sgit", "foo", "../foo.git"],
On 2014/07/31 20:01:54, nweiz wrote:
> Now I'm really confused. Where's the ref? What's this "foo" doing here?

Killing git support in this patch.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
File sdk/lib/_internal/pub/test/global/deactivate/remote_package_test.dart
(right):

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/deactivate/remote_package_test.dart:1: //
Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
On 2014/07/31 20:01:54, nweiz wrote:
> Nit: it seems like this should be called hosted_package_test.

Done.

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
File
sdk/lib/_internal/pub/test/global/run/reflects_changes_to_local_package_test.dart
(right):

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/run/reflects_changes_to_local_package_test.dart:10:
integration('runs a script in a local activated package', () {
On 2014/07/31 20:01:54, nweiz wrote:
> This should match the test file name.

Done.

[nweiz, 2014-08-05 19:14:42]

lgtm

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:196: "version
${log.bold(id.version)}.");
On 2014/08/04 23:41:50, Bob Nystrom wrote:
> On 2014/07/31 20:01:53, nweiz wrote:
> > "is already active" sounds to me like the sort of thing you'd see in an
error
> > message describing why activation failed. Maybe "was active" instead to make
> it
> > clear that it's being replaced with the new source/version?
> 
> Putting it in the past tense may be confusing if activating the new package
> fails. Changed to "is currently active". Is that better?

SGTM

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/does_not_install_dependencies_for_path_test.dart:23:
output: "Activated foo 1.0.0.");
On 2014/08/04 23:41:51, Bob Nystrom wrote:
> On 2014/07/31 20:01:54, nweiz wrote:
> > This doesn't seem like the behavior users would want. It seems like, if the
> > dependencies haven't yet been installed for a path package, they should be
> when
> > that package is activated.
> 
> When you activate a package at a path, it relies on the lock file in that
> directory. I felt like implicitly running pub get in *that* directory when you
> want pub global activate wasn't the behavior users would expect, but if we
don't
> do that, we can't install the dependencies.
> 
> In practice, I don't think it matters much: most users activating a package at
a
> path will already have a lockfile which implies they'll already have the
> dependencies. If they don't, pub global run *will* implicitly run pub get.
> 
> I think it's useful for pub global activate to eagerly pull down dependencies
> when you activate remote (hosted/git) packages because you may be about to go
> offline. But for path packages, you probably have the deps already and/or know
> what you're doing, so it's less important to do the get eagerly.
> 
> You're right that this is a semantically strange corner case. I added this
test
> specifically to track the current behavior since I didn't realize it at first
> either. If you think it makes sense to implicitly run pub get on the activated
> directory, I could be persuaded.

I think it does. Think about what happens with the current behavior if you run
"pub global run" after activating a path package. Either it will install the
dependencies before running, in which case that's very similar to just
installing them eagerly when the package is activated; or it will break due to
lack of dependencies, which really hoses the use case of installing a package
from a tarfile (or whatever).

[Bob Nystrom, 2014-08-06 20:19:57]

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

[Bob Nystrom, 2014-08-06 20:21:14]

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

https://codereview.chromium.org/428313004/diff/1/sdk/lib/_internal/pub/test/g...
sdk/lib/_internal/pub/test/global/activate/does_not_install_dependencies_for_path_test.dart:23:
output: "Activated foo 1.0.0.");
On 2014/08/05 19:14:42, nweiz wrote:
> On 2014/08/04 23:41:51, Bob Nystrom wrote:
> > On 2014/07/31 20:01:54, nweiz wrote:
> > > This doesn't seem like the behavior users would want. It seems like, if
the
> > > dependencies haven't yet been installed for a path package, they should be
> > when
> > > that package is activated.
> > 
> > When you activate a package at a path, it relies on the lock file in that
> > directory. I felt like implicitly running pub get in *that* directory when
you
> > want pub global activate wasn't the behavior users would expect, but if we
> don't
> > do that, we can't install the dependencies.
> > 
> > In practice, I don't think it matters much: most users activating a package
at
> a
> > path will already have a lockfile which implies they'll already have the
> > dependencies. If they don't, pub global run *will* implicitly run pub get.
> > 
> > I think it's useful for pub global activate to eagerly pull down
dependencies
> > when you activate remote (hosted/git) packages because you may be about to
go
> > offline. But for path packages, you probably have the deps already and/or
know
> > what you're doing, so it's less important to do the get eagerly.
> > 
> > You're right that this is a semantically strange corner case. I added this
> test
> > specifically to track the current behavior since I didn't realize it at
first
> > either. If you think it makes sense to implicitly run pub get on the
activated
> > directory, I could be persuaded.
> 
> I think it does. Think about what happens with the current behavior if you run
> "pub global run" after activating a path package. Either it will install the
> dependencies before running, in which case that's very similar to just
> installing them eagerly when the package is activated; or it will break due to
> lack of dependencies, which really hoses the use case of installing a package
> from a tarfile (or whatever).

I'm convinced. I realized that pub build and serve will implicitly run pub
get--including overwriting your lockfile too--so it doesn't seem inconsistent to
do so here.

Done.