Support running executables from dependencies.

Support running executables from dependencies.

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

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

[nweiz, 2014-06-19 20:12:11]

I'd like to see a test that running an executable in a transitive dependency
fails if it's not a direct dependency. Also probably a test that running an
executable from a dev dependency works, and a test that running an executable
from a nonexistent dependency fails in a reasonable way.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:211: /// to the
bound server.
Explain the difference between this and [serveDirectory].

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:408: return
BarbackServer.bind(this, _hostname, 0, rootPackage.name, null)
It's a little confusing that [rootDirectory] is null here. It would feel less
ad-hoc if you made it an optional argument, but either way document what it
means for it to be null in the [BarbackServer.bind] docs.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:50: /// the root
directory.
Document that [rootDirectory] is relative to [package]'s root.

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:52: String host, int
port, String package, String rootDirectory) {
It might also be nice to make [package] optional.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:74: usageError("Can not run an
executable in a subdirectory of a "
I suggest 'Cannot run executable "$script" in a subdirectory of dependency
"$package".' just to make it clear how we parsed the argument.

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:102: var mode = "--checked";
I think we should default to unchecked mode for dependency executables.

[Bob Nystrom, 2014-06-20 00:14:20]

> I'd like to see a test that running an executable in a transitive dependency
fails if it's not a direct dependency. Also probably a test that running an
executable from a dev dependency works, and a test that running an executable
from a nonexistent dependency fails in a reasonable way.

Great ideas. Done.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:211: /// to the
bound server.
On 2014/06/19 20:12:10, nweiz wrote:
> Explain the difference between this and [serveDirectory].

Done.

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/asset_environment.dart:408: return
BarbackServer.bind(this, _hostname, 0, rootPackage.name, null)
On 2014/06/19 20:12:10, nweiz wrote:
> It's a little confusing that [rootDirectory] is null here. It would feel less
> ad-hoc if you made it an optional argument, but either way document what it
> means for it to be null in the [BarbackServer.bind] docs.

Made it named and explained its omission.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:50: /// the root
directory.
On 2014/06/19 20:12:10, nweiz wrote:
> Document that [rootDirectory] is relative to [package]'s root.

Done.

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:52: String host, int
port, String package, String rootDirectory) {
On 2014/06/19 20:12:10, nweiz wrote:
> It might also be nice to make [package] optional.

Done.

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:74: usageError("Can not run an
executable in a subdirectory of a "
On 2014/06/19 20:12:10, nweiz wrote:
> I suggest 'Cannot run executable "$script" in a subdirectory of dependency
> "$package".' just to make it clear how we parsed the argument.

I agree the error message here isn't great, but I think your suggestion is
confusing. It makes it sound like "foo/bar" is being run in some *other*
(unspecified) subdirectory, and not that "foo/bar" itself *is* the subdirectory.
Likewise, showing the package name might lead the user to be believe there's
something wrong with trying to do the command against that package in
particular.

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:102: var mode = "--checked";
On 2014/06/19 20:12:10, nweiz wrote:
> I think we should default to unchecked mode for dependency executables.

I'm really hesitant to add *any* policy or logic here because it just adds
complexity and confuses people. Personally, I think the best option would be to
run everything unchecked and let people do "pub run --checked foo" if they want
it to be checked.

I'm not a fan of unchecked mode, but that's probably less surprising to users. I
totally get the argument of wanting to run tests checked, but I think that's
something people can just get in the habit of doing.

[nweiz, 2014-06-20 02:08:28]

lgtm

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

https://codereview.chromium.org/341123002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:102: var mode = "--checked";
On 2014/06/20 00:14:20, Bob Nystrom wrote:
> On 2014/06/19 20:12:10, nweiz wrote:
> > I think we should default to unchecked mode for dependency executables.
> 
> I'm really hesitant to add *any* policy or logic here because it just adds
> complexity and confuses people. Personally, I think the best option would be
to
> run everything unchecked and let people do "pub run --checked foo" if they
want
> it to be checked.
> 
> I'm not a fan of unchecked mode, but that's probably less surprising to users.
I
> totally get the argument of wanting to run tests checked, but I think that's
> something people can just get in the habit of doing.

I disagree. I think most users will run their executables with the attitude of
"figuring out the correct configuration is hard; the system probably has good
defaults." The path of least resistance is them not thinking about whether or
not they want checked mode, so they won't use it and their tests just won't
catch a bunch of errors that they otherwise would.

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

https://codereview.chromium.org/341123002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:49: /// This server
serves assets from [barback], and use [rootDirectory]
"use" -> "uses"

https://codereview.chromium.org/341123002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:59: log.fine('Bound
package serving to $host:$port.');
I think "package: serving" might be a little clearer here.

[Sean Eagan, 2014-06-20 12:42:39]

DBC

https://chromiumcodereview.appspot.com/341123002/diff/20001/sdk/lib/_internal...
File sdk/lib/_internal/pub/lib/src/command/run.dart (right):

https://chromiumcodereview.appspot.com/341123002/diff/20001/sdk/lib/_internal...
sdk/lib/_internal/pub/lib/src/command/run.dart:62: if (script.contains(":")) {
Instead of inventing a syntax for this:

    pub run <package>:<path> [args...]

why not just use normal cli arguments:

    pub run [--package=<package>] <path> [args...]

?

[Bob Nystrom, 2014-06-20 17:35:06]

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

[Bob Nystrom, 2014-06-20 17:35:27]

Thanks!

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

https://codereview.chromium.org/341123002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:49: /// This server
serves assets from [barback], and use [rootDirectory]
On 2014/06/20 02:08:28, nweiz wrote:
> "use" -> "uses"

Done.

https://codereview.chromium.org/341123002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/barback_server.dart:59: log.fine('Bound
package serving to $host:$port.');
On 2014/06/20 02:08:28, nweiz wrote:
> I think "package: serving" might be a little clearer here.

Reworded to something better. What it binds is "package serving", so the colon
would be weird there.

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

https://codereview.chromium.org/341123002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/run.dart:62: if (script.contains(":")) {
On 2014/06/20 12:42:38, Sean Eagan wrote:
> Instead of inventing a syntax for this:
> 
>     pub run <package>:<path> [args...]
> 
> why not just use normal cli arguments:
> 
>     pub run [--package=<package>] <path> [args...]
> 
> ?

My hunch is that this will be a very common use case, so I wanted the syntax to
be lightweight. Something like:

$ pub run foo:bar arg arg

Having to do:

$ pub run --package foo bar arg arg

feels pretty painful to me.