Create binstubs for executables when activating a package.

Create binstubs for executables when activating a package.

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

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

[Bob Nystrom, 2014-09-13 00:36:51]

rnystrom@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2014-09-15 22:57:18]

Right now this doesn't really do anything helpful if the package defines an
executable that references a nonexistent script. I think it should warn if a
package with such an executable is published and print an error message if it's
activated.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
This is a really long and unwieldy flag name. I like just "force" here.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:68: createBinStubs:
binstubs, overwriteBinStubs: overwrite);
Rather than having separate [executables] and [createBinStubs] arguments here,
maybe just have [executables] which can be null (indicating the default
executables), an empty list (indicating no executables), or a populated list
(indicating specific executables).

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:80: /// [createBinStubs] is
false.
Document [overwriteBinStubs].

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:95: overwriteBinStubs:
overwriteBinStubs);
Why not just have [_installInCache] call [_updateBinStubs] itself?

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:423: for (var command in
allExecutables) {
Nit: "command" -> "executable" for consistency.

This method is pretty long. Maybe move the contents of this for loop into a
subroutine?

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:454: pub global run
${package.name}:$script %*
Add a TODO to run the snapshot directly if possible.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:464: pub global run
${package.name}:$script \$@
$@ should be quoted for stupid bash reasons. If you don't quote it, running
`script "foo bar"` will end up running `pub global run script:script foo bar`
rather than `pub global run script:script "foo bar"`.

I'm not sure if the same is true for %* in Windows, but it's worth trying to
find out.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:471: fail('Could not make
"$binStubPath" executable:\n${result.stderr}');
Include the exit code here.

I think this should be an error. If the executable isn't actually executable, it
may as well not exist.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:498:
"${log.bold(package.name)} using --force.");
If you don't rename "--overwrite-executables" to "--force", use the former here.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:518: // .DS_Store on Mac.
...not that hidden files will be listed by [listDir] anyway. I think we should
just let these hit [log.fine] below.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:242: /// The executables that should
be placed on the user's PATH when this
"placed on the user's PATH" isn't really accurate, since we're putting them in a
directory that probably won't be on the user's PATH by default.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:249: /// strings: alphanumerics,
underscores and hypens only. If a value is
We don't otherwise restrict the name of executable scripts, do we? It seems a
little weird to do so here.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:176: if (iter.length == 1) return
"$name ${iter.first}";
Nit: I like [iter.single] here.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:178: var sequence =
iter.take(iter.length - 1).join(", ") + " and ${iter.last}";
Use [toSentence] here.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pubspec_test.dart (right):

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pubspec_test.dart:484:
expect(pubspec.executables['command'], isNull);
I feel like this should normalize to return the command name rather than null.

[Bob Nystrom, 2014-09-17 21:34:35]

> Right now this doesn't really do anything helpful if the package defines an
executable that references a nonexistent script. I think it should warn if a
package with such an executable is published and print an error message if it's
activated.

Done.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
On 2014/09/15 22:57:17, nweiz wrote:
> This is a really long and unwieldy flag name. I like just "force" here.

I did that at first but I felt like "force" was ambiguous. It's clear "force" is
pushing past *some* error, but not what kind. (For example, should --force
ignore errors from unknown executables?)

I went with this since it's explicit which errors are being ignored. I don't
think it will be used in practice much, so I'm OK with it being long.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:68: createBinStubs:
binstubs, overwriteBinStubs: overwrite);
On 2014/09/15 22:57:17, nweiz wrote:
> Rather than having separate [executables] and [createBinStubs] arguments here,
> maybe just have [executables] which can be null (indicating the default
> executables), an empty list (indicating no executables), or a populated list
> (indicating specific executables).

Done.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:80: /// [createBinStubs] is
false.
On 2014/09/15 22:57:18, nweiz wrote:
> Document [overwriteBinStubs].

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:95: overwriteBinStubs:
overwriteBinStubs);
On 2014/09/15 22:57:17, nweiz wrote:
> Why not just have [_installInCache] call [_updateBinStubs] itself?

I thought about that, but it felt pointless to pipe [executables] and
[overwriteBinStubs] through it just to pass them on to _updateBinStubs,
especially when there are only two callsites for this.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:423: for (var command in
allExecutables) {
On 2014/09/15 22:57:17, nweiz wrote:
> Nit: "command" -> "executable" for consistency.
> 
> This method is pretty long. Maybe move the contents of this for loop into a
> subroutine?

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:454: pub global run
${package.name}:$script %*
On 2014/09/15 22:57:17, nweiz wrote:
> Add a TODO to run the snapshot directly if possible.

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:464: pub global run
${package.name}:$script \$@
On 2014/09/15 22:57:17, nweiz wrote:
> $@ should be quoted for stupid bash reasons. If you don't quote it, running
> `script "foo bar"` will end up running `pub global run script:script foo bar`
> rather than `pub global run script:script "foo bar"`.
> 
> I'm not sure if the same is true for %* in Windows, but it's worth trying to
> find out.

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:471: fail('Could not make
"$binStubPath" executable:\n${result.stderr}');
On 2014/09/15 22:57:17, nweiz wrote:
> Include the exit code here.

Done.

> I think this should be an error. If the executable isn't actually executable,
it
> may as well not exist.

fail() accomplishes that, doesn't it?

I did change this to delete the binstub if it couldn't make it executable. In
practice, I don't know if this will be a common problem.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:498:
"${log.bold(package.name)} using --force.");
On 2014/09/15 22:57:17, nweiz wrote:
> If you don't rename "--overwrite-executables" to "--force", use the former
here.

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/global_packages.dart:518: // .DS_Store on Mac.
On 2014/09/15 22:57:18, nweiz wrote:
> ...not that hidden files will be listed by [listDir] anyway. I think we should
> just let these hit [log.fine] below.

Done.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:249: /// strings: alphanumerics,
underscores and hypens only. If a value is
On 2014/09/15 22:57:18, nweiz wrote:
> We don't otherwise restrict the name of executable scripts, do we?

The OS implicitly prevents you from putting path separators in a file name,
which was my initial concern here. Restricting it beyond that seemed like good
PATH hygiene to me. Since activating a package will put all of its executables
in the global namespace, I want them to be "nice" from the consumer's
perspective.

We can loosen these guidelines over time, but we can't tighten them, so I
figured we could start conservatively.

Also, it makes it clear that the ".dart" isn't needed for the script.

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:176: if (iter.length == 1) return
"$name ${iter.first}";
On 2014/09/15 22:57:18, nweiz wrote:
> Nit: I like [iter.single] here.

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:178: var sequence =
iter.take(iter.length - 1).join(", ") + " and ${iter.last}";
On 2014/09/15 22:57:18, nweiz wrote:
> Use [toSentence] here.

Done.

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/test/p...
File sdk/lib/_internal/pub/test/pubspec_test.dart (right):

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/test/p...
sdk/lib/_internal/pub/test/pubspec_test.dart:484:
expect(pubspec.executables['command'], isNull);
On 2014/09/15 22:57:18, nweiz wrote:
> I feel like this should normalize to return the command name rather than null.

Done.

[nweiz, 2014-09-17 22:58:27]

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
On 2014/09/17 21:34:34, Bob Nystrom wrote:
> On 2014/09/15 22:57:17, nweiz wrote:
> > This is a really long and unwieldy flag name. I like just "force" here.
> 
> I did that at first but I felt like "force" was ambiguous. It's clear "force"
is
> pushing past *some* error, but not what kind. (For example, should --force
> ignore errors from unknown executables?)
> 
> I went with this since it's explicit which errors are being ignored. I don't
> think it will be used in practice much, so I'm OK with it being long.

What do you think about just "--overwrite"?

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:249: /// strings: alphanumerics,
underscores and hypens only. If a value is
On 2014/09/17 21:34:35, Bob Nystrom wrote:
> On 2014/09/15 22:57:18, nweiz wrote:
> > We don't otherwise restrict the name of executable scripts, do we?
> 
> The OS implicitly prevents you from putting path separators in a file name,
> which was my initial concern here. Restricting it beyond that seemed like good
> PATH hygiene to me. Since activating a package will put all of its executables
> in the global namespace, I want them to be "nice" from the consumer's
> perspective.
> 
> We can loosen these guidelines over time, but we can't tighten them, so I
> figured we could start conservatively.
> 
> Also, it makes it clear that the ".dart" isn't needed for the script.

I don't mind the restrictions on the binstub name, but it seems very strange to
me that the user could create an executable named "my.script.dart" and it would
work fine locally, work fine when run as a dependency executable, but fail if
they tried to make it a global executable, *even though they're aliasing it
anyway*.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:73: return
globals.activateGit(repo, executables,
Nit: I'd probably make [executables] named here and for the other activate
methods, so that the semantics of not passing it matches the semantics of not
passing --executable on the command line.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:496: if
(!binFiles.contains("bin/$script.dart")) {
Use p.join here.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:146: /// which is expected to be
relative to the package's root directory. If
I think you missed an update for this in [Package.executableIds].

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:30: if
(!binFiles.contains("bin/$script.dart")) {
Use p.join here. Also maybe short-circuit rather than nesting.

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:32: 'points to a script
"bin/$script.dart" that does not exist.');
Use p.join here as well to give native-feeling paths.

[Bob Nystrom, 2014-09-18 19:17:53]

Thanks!

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
On 2014/09/17 22:58:26, nweiz wrote:
> On 2014/09/17 21:34:34, Bob Nystrom wrote:
> > On 2014/09/15 22:57:17, nweiz wrote:
> > > This is a really long and unwieldy flag name. I like just "force" here.
> > 
> > I did that at first but I felt like "force" was ambiguous. It's clear
"force"
> is
> > pushing past *some* error, but not what kind. (For example, should --force
> > ignore errors from unknown executables?)
> > 
> > I went with this since it's explicit which errors are being ignored. I don't
> > think it will be used in practice much, so I'm OK with it being long.
> 
> What do you think about just "--overwrite"?

Maybe? I'm worried that that isn't clear. The command the user is running is
"pub global activate" which isn't itself specifically about binstubs, so maybe
the user would think that overwrites the package itself (whatever that would
mean).

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/pubspec.dart:249: /// strings: alphanumerics,
underscores and hypens only. If a value is
On 2014/09/17 22:58:26, nweiz wrote:
> On 2014/09/17 21:34:35, Bob Nystrom wrote:
> > On 2014/09/15 22:57:18, nweiz wrote:
> > > We don't otherwise restrict the name of executable scripts, do we?
> > 
> > The OS implicitly prevents you from putting path separators in a file name,
> > which was my initial concern here. Restricting it beyond that seemed like
good
> > PATH hygiene to me. Since activating a package will put all of its
executables
> > in the global namespace, I want them to be "nice" from the consumer's
> > perspective.
> > 
> > We can loosen these guidelines over time, but we can't tighten them, so I
> > figured we could start conservatively.
> > 
> > Also, it makes it clear that the ".dart" isn't needed for the script.
> 
> I don't mind the restrictions on the binstub name, but it seems very strange
to
> me that the user could create an executable named "my.script.dart" and it
would
> work fine locally, work fine when run as a dependency executable, but fail if
> they tried to make it a global executable, *even though they're aliasing it
> anyway*.

Done.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:73: return
globals.activateGit(repo, executables,
On 2014/09/17 22:58:26, nweiz wrote:
> Nit: I'd probably make [executables] named here and for the other activate
> methods, so that the semantics of not passing it matches the semantics of not
> passing --executable on the command line.

If there were callsites that didn't pass that, I would. But since it is provided
in all cases, I think this is simpler.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/global_packages.dart:496: if
(!binFiles.contains("bin/$script.dart")) {
On 2014/09/17 22:58:27, nweiz wrote:
> Use p.join here.

Done.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/package.dart:146: /// which is expected to be
relative to the package's root directory. If
On 2014/09/17 22:58:27, nweiz wrote:
> I think you missed an update for this in [Package.executableIds].

Done.

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

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:30: if
(!binFiles.contains("bin/$script.dart")) {
On 2014/09/17 22:58:27, nweiz wrote:
> Use p.join here. Also maybe short-circuit rather than nesting.

Done.

https://codereview.chromium.org/566093003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:32: 'points to a script
"bin/$script.dart" that does not exist.');
On 2014/09/17 22:58:27, nweiz wrote:
> Use p.join here as well to give native-feeling paths.

Done.

[nweiz, 2014-09-18 20:47:46]

lgtm

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
On 2014/09/18 19:17:52, Bob Nystrom wrote:
> On 2014/09/17 22:58:26, nweiz wrote:
> > On 2014/09/17 21:34:34, Bob Nystrom wrote:
> > > On 2014/09/15 22:57:17, nweiz wrote:
> > > > This is a really long and unwieldy flag name. I like just "force" here.
> > > 
> > > I did that at first but I felt like "force" was ambiguous. It's clear
> "force"
> > is
> > > pushing past *some* error, but not what kind. (For example, should --force
> > > ignore errors from unknown executables?)
> > > 
> > > I went with this since it's explicit which errors are being ignored. I
don't
> > > think it will be used in practice much, so I'm OK with it being long.
> > 
> > What do you think about just "--overwrite"?
> 
> Maybe? I'm worried that that isn't clear. The command the user is running is
> "pub global activate" which isn't itself specifically about binstubs, so maybe
> the user would think that overwrites the package itself (whatever that would
> mean).

I think in practice most of the time users will be running "pub global activate"
specifically to get access to the executables of packages they're activating,
and that will provide enough context. Regardless, I think terseness should
general beat perfect clarity for command-line flags.

[nweiz, 2014-09-18 21:51:49]

https://codereview.chromium.org/566093003/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/validator/executable.dart (right):

https://codereview.chromium.org/566093003/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:5: library
pub.validator.compiled_dartdoc;
One more thing: make this match the path.

[Bob Nystrom, 2014-09-18 22:23:33]

Committed patchset #3 (id:40001) manually as 40471 (presubmit successful).

[Bob Nystrom, 2014-09-18 22:23:48]

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

https://codereview.chromium.org/566093003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/global_activate.dart:33:
commandParser.addFlag("overwrite-executables", negatable: false,
On 2014/09/18 20:47:46, nweiz wrote:
> On 2014/09/18 19:17:52, Bob Nystrom wrote:
> > On 2014/09/17 22:58:26, nweiz wrote:
> > > On 2014/09/17 21:34:34, Bob Nystrom wrote:
> > > > On 2014/09/15 22:57:17, nweiz wrote:
> > > > > This is a really long and unwieldy flag name. I like just "force"
here.
> > > > 
> > > > I did that at first but I felt like "force" was ambiguous. It's clear
> > "force"
> > > is
> > > > pushing past *some* error, but not what kind. (For example, should
--force
> > > > ignore errors from unknown executables?)
> > > > 
> > > > I went with this since it's explicit which errors are being ignored. I
> don't
> > > > think it will be used in practice much, so I'm OK with it being long.
> > > 
> > > What do you think about just "--overwrite"?
> > 
> > Maybe? I'm worried that that isn't clear. The command the user is running is
> > "pub global activate" which isn't itself specifically about binstubs, so
maybe
> > the user would think that overwrites the package itself (whatever that would
> > mean).
> 
> I think in practice most of the time users will be running "pub global
activate"
> specifically to get access to the executables of packages they're activating,
> and that will provide enough context.

Good point. Done.

https://codereview.chromium.org/566093003/diff/40001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/validator/executable.dart (right):

https://codereview.chromium.org/566093003/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/validator/executable.dart:5: library
pub.validator.compiled_dartdoc;
On 2014/09/18 21:51:49, nweiz wrote:
> One more thing: make this match the path.

Done.