Add a "pub global run" command.

Add a "pub global run" command.

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=37963

[nweiz, 2014-07-02 00:12:03]

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_run.dart:33: if
(commandOptions.rest.isEmpty) {
I had assumed that "pub global run" would use the same global executable names
that would be added to PATH.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:56: /// Creates an entrypoint
given a package and lockfile.
"package and lockfile" -> "package, lockfile, and cache".

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/exceptions.dart:59: String toString() =>
"$message\n\n$_usage";
Gracefully handle usage not being bound.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:23: /// The [executable] is a
reference to an "executable", a relative Dart file
"The" here is confusing, since we generally don't use the definite article for
parameters in general and you already referred to [executable] without it in the
previous sentence. I know you don't like starting paragraphs with variable
names, but I don't think this is the right way to avoid that here.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:26: /// package, or an immediate
dependency of it.
Nix this comma.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:40: usageError("Can not run an
executable in a subdirectory of a "
"Can not" -> "Cannot"

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:61: // implicitly by
getAllAssets().
This comment doesn't apply here.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:71: } else {
Remove this; you can just short-circuit here.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:102: from:
p.url.joinAll(p.split(server.rootDirectory)));
Indent this line.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:136: on IOException catch
(error) {
Nit: move this to the previous line.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:143: return
_source.downloadToSystemCache(id);
It reads very weird that this is "downloadToSystemCache" despite the fact that
we neither expect nor want it to download anything.

Why aren't you using [Source.getDirectory] here?

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:414:
pub.stdout.expect(startsWith("Loading $transformer transformers"));
Rather than requiring tests to pass in a list of transformers, maybe just do

    pub.stdout.expect(consumeWhile(startsWith("Loading ")));

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:798: "block.");
Fix this indentation.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:807:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
Also this indentation.

[Sean Eagan, 2014-07-02 15:57:16]

On 2014/07/01 17:54:45, Bob Nystrom wrote:

So this allows us to run executables when cwd is outside of any package:

    pub global activate foo
    pub global run foo bar --x y z

Why would I want to do that instead of just:

    pub global install foo bar
    bar --x y z

I guess the main reason is if I want to avoid polluting my PATH with 'bar', but
I would say installing directly to PATH is the ~90% use case here.  It could
support similar things as dart's import syntax:

    pub global install foo #installs each executable by default
    foo -ab c
    bar --x y z

    pub global install foo --show bar
    bar --x y z

    pub global install foo --hide bar
    foo -ab c

    pub global install foo --as foo_
    foo_foo -ab c
    foo_bar --x y z

Also:

    pub global run <package> <executable>

seems inconsistent with:

    pub run <package>:<executable>

one contains : other does not.

[Sean Eagan, 2014-07-02 16:20:15]

On 2014/07/02 15:57:16, Sean Eagan wrote:
> On 2014/07/01 17:54:45, Bob Nystrom wrote:
> 
> So this allows us to run executables when cwd is outside of any package:
> 
>     pub global activate foo
>     pub global run foo bar --x y z
> 
> Why would I want to do that instead of just:
> 
>     pub global install foo --show bar
>     bar --x y z
> 
> I guess the main reason is if I want to avoid polluting my PATH with 'bar'

And if it's easy to uninstall that helps alleviate this concern:

    pub global uninstall foo

[Bob Nystrom, 2014-07-02 18:00:53]

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_run.dart:33: if
(commandOptions.rest.isEmpty) {
On 2014/07/02 00:12:02, nweiz wrote:
> I had assumed that "pub global run" would use the same global executable names
> that would be added to PATH.

I wanted something simpler and more explicit for this. Since the stuff on your
PATH is in a different namespace (i.e. set of other named things the user may be
referring to) than the things you can pass to "pub global run", I think it makes
sense to have different strategies there.

We can discuss this more if you'd like.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:56: /// Creates an entrypoint
given a package and lockfile.
On 2014/07/02 00:12:02, nweiz wrote:
> "package and lockfile" -> "package, lockfile, and cache".

In all cases, it needs a cache, so I don't think it adds much to mention it
here. Reworded to stress that this is for creating an entrypoint based on
existing objects.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/exceptions.dart:59: String toString() =>
"$message\n\n$_usage";
On 2014/07/02 00:12:02, nweiz wrote:
> Gracefully handle usage not being bound.

Done.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:23: /// The [executable] is a
reference to an "executable", a relative Dart file
On 2014/07/02 00:12:02, nweiz wrote:
> "The" here is confusing, since we generally don't use the definite article for
> parameters in general and you already referred to [executable] without it in
the
> previous sentence. I know you don't like starting paragraphs with variable
> names, but I don't think this is the right way to avoid that here.

Reworded.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:26: /// package, or an immediate
dependency of it.
On 2014/07/02 00:12:02, nweiz wrote:
> Nix this comma.

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:40: usageError("Can not run an
executable in a subdirectory of a "
On 2014/07/02 00:12:02, nweiz wrote:
> "Can not" -> "Cannot"

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:61: // implicitly by
getAllAssets().
On 2014/07/02 00:12:02, nweiz wrote:
> This comment doesn't apply here.

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:71: } else {
On 2014/07/02 00:12:02, nweiz wrote:
> Remove this; you can just short-circuit here.

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:102: from:
p.url.joinAll(p.split(server.rootDirectory)));
On 2014/07/02 00:12:02, nweiz wrote:
> Indent this line.

Done.

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:136: on IOException catch
(error) {
On 2014/07/02 00:12:02, nweiz wrote:
> Nit: move this to the previous line.

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:143: return
_source.downloadToSystemCache(id);
On 2014/07/02 00:12:02, nweiz wrote:
> It reads very weird that this is "downloadToSystemCache" despite the fact that
> we neither expect nor want it to download anything.
> 
> Why aren't you using [Source.getDirectory] here?

downloadToSystemCache conveniently handles loading the package from the
directory, but this is confusing, so changed it.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:414:
pub.stdout.expect(startsWith("Loading $transformer transformers"));
On 2014/07/02 00:12:03, nweiz wrote:
> Rather than requiring tests to pass in a list of transformers, maybe just do
> 
>     pub.stdout.expect(consumeWhile(startsWith("Loading ")));

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:798: "block.");
On 2014/07/02 00:12:03, nweiz wrote:
> Fix this indentation.

Done.

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/test_pub.dart:807:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
On 2014/07/02 00:12:02, nweiz wrote:
> Also this indentation.

Done.

[nweiz, 2014-07-02 19:57:50]

lgtm

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

https://codereview.chromium.org/354763006/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_run.dart:33: if
(commandOptions.rest.isEmpty) {
On 2014/07/02 18:00:52, Bob Nystrom wrote:
> On 2014/07/02 00:12:02, nweiz wrote:
> > I had assumed that "pub global run" would use the same global executable
names
> > that would be added to PATH.
> 
> I wanted something simpler and more explicit for this. Since the stuff on your
> PATH is in a different namespace (i.e. set of other named things the user may
be
> referring to) than the things you can pass to "pub global run", I think it
makes
> sense to have different strategies there.
> 
> We can discuss this more if you'd like.

I think this behavior is fine, it just took me by surprise a bit.

[Bob Nystrom, 2014-07-02 23:12:15]

On 2014/07/02 15:57:16, Sean Eagan wrote:
> On 2014/07/01 17:54:45, Bob Nystrom wrote:
> So this allows us to run executables when cwd is outside of any package:
> 
>     pub global activate foo
>     pub global run foo bar --x y z
> 
> Why would I want to do that instead of just:
> 
>     pub global install foo bar
>     bar --x y z

Yes, this is what's most user friendly. We'll be working on that soon. "pub
global run" is sort of the plumbing that that will sit on top of. It also has
the advantage of being less prone to name collisions.

> I guess the main reason is if I want to avoid polluting my PATH with 'bar',
but
> I would say installing directly to PATH is the ~90% use case here. 

Yup, agreed. Putting stuff on the path will be nice. But a package may want to
expose a few executables that are too niche to be worth putting on the user's
path.

Also, the way it will work is that we'll generate little shell scripts that run
the right executable. They'll do that by in turn just invoking "pub global run
...".

>     pub global install foo #installs each executable by default
>     pub global install foo --show bar
>     pub global install foo --hide bar
>     pub global install foo --as foo_

Ah, this is an interesting idea. Figuring out how the user controls which
executables actually end up on their PATH is one of the things we still have to
decide.

> Also:
> 
>     pub global run <package> <executable>
> 
> seems inconsistent with:
> 
>     pub run <package>:<executable>
> 
> one contains : other does not.

Yeah, I thought about that, but since the package name is mandatory, it felt a
little strange to use ":" like pub run does, where it's optional. Do you think
":" would be better, even though it's required?

- bob

[Bob Nystrom, 2014-07-02 23:12:49]

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

[Sean Eagan, 2014-07-03 18:37:43]

On 2014/07/02 23:12:15, Bob Nystrom wrote:
> Yup, agreed. Putting stuff on the path will be nice. But a package may want to
> expose a few executables that are too niche to be worth putting on the user's
> path.

When we also have commands to add/remove executables from the PATH (e.g. "global
install" and "global uninstall"), my main concern is the complicated life-cycle
of "activate", "install", "uninstall", "deactivate".  It would be nice to
combine "activate"+"install" and "uninstall"+"deactivate".

Strawman:

    pub global install [options] <package> <source>

    Installs executables to PATH and makes them available via 'pub global run'.
    By default all executables are installed to PATH, but see [options].

    <package> <source> corresponds to "dependencies: <package>: <source>" in a
pubspec.yaml.
    Thus, <source> is a yaml snippet.

Options:

    --as=<name>                Name to prefix executables in PATH with:
                                   <name>-<executable>
                               and to identify the package with in later
commands:
                                   pub global run <name> <executable>
                                   pub global uninstall <name>
                               Allows for multiple packages installed with same
name.
                               (Defaults to <package>)

    --show=<executables>       Show <executables> in PATH, hide all others. 
Comma separated list.
    --hide=<executables>       Hide <executables> from PATH, show all others. 
Comma separated list.
    --hide-all                 Hides all executables from PATH.  They will only
be available via
                               'pub global run'.

Examples:

    pub global install foo
    pub global install foo ">1.0.0 < 2.0.0"
    pub global install foo "path: /path/to/foo"
    pub global install foo "git: .../foo.git"
    pub global install foo --show foo
    pub global install foo --hide bar
    pub global install foo --hide-all    
    pub global uninstall foo

    pub global install foo --as dev
    dev-foo
    pub global run dev foo
    pub global uninstall dev

> > Also:
> > 
> >     pub global run <package> <executable>
> > 
> > seems inconsistent with:
> > 
> >     pub run <package>:<executable>
> > 
> > one contains : other does not.
> 
> Yeah, I thought about that, but since the package name is mandatory, it felt a
> little strange to use ":" like pub run does, where it's optional. Do you think
> ":" would be better, even though it's required?

I'd actually prefer both without ":" (pub run would use --package), but barring
that, yeah I think it feels weird if they are different.

[nweiz, 2014-07-07 19:23:33]

I believe the plan is to have "activate" add the package's executables to the
path automatically. I don't think there'll be a separate command for that.

On 2014/07/03 18:37:43, Sean Eagan wrote:
> On 2014/07/02 23:12:15, Bob Nystrom wrote:
> > Yup, agreed. Putting stuff on the path will be nice. But a package may want
to
> > expose a few executables that are too niche to be worth putting on the
user's
> > path.
> 
> When we also have commands to add/remove executables from the PATH (e.g.
"global
> install" and "global uninstall"), my main concern is the complicated
life-cycle
> of "activate", "install", "uninstall", "deactivate".  It would be nice to
> combine "activate"+"install" and "uninstall"+"deactivate".
> 
> Strawman:
> 
>     pub global install [options] <package> <source>
> 
>     Installs executables to PATH and makes them available via 'pub global
run'.
>     By default all executables are installed to PATH, but see [options].
> 
>     <package> <source> corresponds to "dependencies: <package>: <source>" in a
> pubspec.yaml.
>     Thus, <source> is a yaml snippet.
> 
> Options:
> 
>     --as=<name>                Name to prefix executables in PATH with:
>                                    <name>-<executable>
>                                and to identify the package with in later
> commands:
>                                    pub global run <name> <executable>
>                                    pub global uninstall <name>
>                                Allows for multiple packages installed with
same
> name.
>                                (Defaults to <package>)
> 
>     --show=<executables>       Show <executables> in PATH, hide all others. 
> Comma separated list.
>     --hide=<executables>       Hide <executables> from PATH, show all others. 
> Comma separated list.
>     --hide-all                 Hides all executables from PATH.  They will
only
> be available via
>                                'pub global run'.
> 
> Examples:
> 
>     pub global install foo
>     pub global install foo ">1.0.0 < 2.0.0"
>     pub global install foo "path: /path/to/foo"
>     pub global install foo "git: .../foo.git"
>     pub global install foo --show foo
>     pub global install foo --hide bar
>     pub global install foo --hide-all    
>     pub global uninstall foo
> 
>     pub global install foo --as dev
>     dev-foo
>     pub global run dev foo
>     pub global uninstall dev
> 
> > > Also:
> > > 
> > >     pub global run <package> <executable>
> > > 
> > > seems inconsistent with:
> > > 
> > >     pub run <package>:<executable>
> > > 
> > > one contains : other does not.
> > 
> > Yeah, I thought about that, but since the package name is mandatory, it felt
a
> > little strange to use ":" like pub run does, where it's optional. Do you
think
> > ":" would be better, even though it's required?
> 
> I'd actually prefer both without ":" (pub run would use --package), but
barring
> that, yeah I think it feels weird if they are different.

[Bob Nystrom, 2014-07-07 21:51:06]

On 2014/07/03 18:37:43, Sean Eagan wrote:
> When we also have commands to add/remove executables from the PATH (e.g.
"global
> install" and "global uninstall"), my main concern is the complicated
life-cycle
> of "activate", "install", "uninstall", "deactivate".  It would be nice to
> combine "activate"+"install" and "uninstall"+"deactivate".

There's no "install" or "uninstall" for global packages. "activate" will
implicitly put the package (and its dependencies) in your cache if they aren't
already there. "deactivate" doesn't remove them from your cache because other
packages may still depend on them.

>     --as=<name>                Name to prefix executables in PATH with:
>                                    <name>-<executable>

Interesting idea! I still have to spend some time thinking and talking to Nathan
about what kind of configurability we want here, but I'll keep this in mind.

>                                    pub global uninstall <name>
>                                Allows for multiple packages installed with
same
> name.

We can't really allow multiple packages with the same name to be globally active
at the same time. Otherwise, there's no way to tell which one the user wants.

>     --show=<executables>       Show <executables> in PATH, hide all others. 
> Comma separated list.
>     --hide=<executables>       Hide <executables> from PATH, show all others. 
> Comma separated list.
>     --hide-all                 Hides all executables from PATH.  They will
only
> be available via
>                                'pub global run'.

This is a good idea. I'm not sure about the details, but some level of user
control over which things end up in their PATH seems important.

>     pub global install foo
>     pub global install foo ">1.0.0 < 2.0.0"
>     pub global install foo "path: /path/to/foo"
>     pub global install foo "git: .../foo.git"

Yeah, supporting sources other than hosted is still a TODO.

> > Yeah, I thought about that, but since the package name is mandatory, it felt
a
> > little strange to use ":" like pub run does, where it's optional. Do you
think
> > ":" would be better, even though it's required?
> 
> I'd actually prefer both without ":" (pub run would use --package), but
barring
> that, yeah I think it feels weird if they are different.

Yeah, I think I'm convinced too. Filed:
https://code.google.com/p/dart/issues/detail?id=19863

Thanks!

- bob