First stab at "pub run" command.

First stab at "pub run" command.

It only runs scripts from the entrypoint package's "bin" directory,
but it's a start.

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

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

[nweiz, 2014-05-29 19:30:08]

I'd also like to see tests for:

* A transformer error.
* A transformer log message.
* Running with --verbose.
* An executable that's entirely generated.
* Whether or not the app runs in checked mode.

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:30: log.showError();
I think we also want to see warnings.

How does this interact with the user explicitly passing --verbose?

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:32: AssetEnvironment environment;
Nit: I don't like typing this, and I don't think it matches our style elsewhere.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:39: // implicitly by
getAllAssets().
What about barback logs? They should be integrated with pub's logging facilities
so they work with the test infrastructure and with --verbose, but we don't want
to silence them with log.showError.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:48: if
(commandOptions.rest.isEmpty) {
This feels weird to me. I get that it's nice to have just "pub run" do something
intuitive, but I really don't like that the user has to do something different
if they want to pass any args.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:56: var scriptPath =
"bin/$command.dart";
Add a TODO to support commands in other directories.

Also, maybe throw an error if [command] contains a path separator, since that
behavior will change in the future.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:68: return
Process.start(Platform.executable, args);
I think this should run in checked mode by default.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
File a bug against dart:io to support "exec()" on applicable platforms and add a
TODO to use it here. Manually piping the streams has several downsides, perhaps
most notably that [stdioType] will return inaccurate results in the inner
process, which means that programs can't safely use colorized output.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:78: log.error("Could not find
$scriptPath.");
[log.fine] the (Chain-ified) stack trace as well.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:80: }, test: (error) => error is
AssetNotFoundException);
Nit: I prefer using "if (error is! AssetNotFoundException)" in the body to using
[test]. It matches our style elsewhere and puts the condition closer to where it
would be if this were a try/catch.

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:230: return callback();
Nit: This looks like it can fit on the previous line.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart (right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart:33:
pub.writeLine("second");
Why aren't you expecting the script to print "first" and "second" here as well?

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/nonexistent_script_test.dart (right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/nonexistent_script_test.dart:11:
integration('runs a Dart application in the entrypoint package', () {
Make the description match the filename.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/runs_transformer_in_entrypoint_test.dart
(right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/runs_transformer_in_entrypoint_test.dart:19:
integration('runs a Dart application in the entrypoint package', () {
Make the description match the filename.

[Bob Nystrom, 2014-05-30 18:17:46]

> * A transformer error.
> * A transformer log message.
> * An executable that's entirely generated.
> * Whether or not the app runs in checked mode.

Done, done, done, and done.

> * Running with --verbose.

The test infrastructure always runs pub with --verbose, so this is implicitly
done by all the tests. (We don't have a test that does *not* do this, but I
don't think it's worth adding.)

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:30: log.showError();
On 2014/05/29 19:30:09, nweiz wrote:
> I think we also want to see warnings.

Done.

> How does this interact with the user explicitly passing --verbose?

This overrides it. Changed it to only mute output if the user does not change
the verbosity.

Either way, it still mutes all pub logging once the spawned process is started.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:32: AssetEnvironment environment;
On 2014/05/29 19:30:09, nweiz wrote:
> Nit: I don't like typing this, and I don't think it matches our style
elsewhere.

Oops, that was a mistake. I sometimes temporarily annotate locals to get
auto-complete to work.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:39: // implicitly by
getAllAssets().
On 2014/05/29 19:30:09, nweiz wrote:
> What about barback logs? They should be integrated with pub's logging
facilities
> so they work with the test infrastructure and with --verbose, but we don't
want
> to silence them with log.showError.

That should be handled by AssetEnvironment, which registers the barback logger.
The behavior here is the same as pub build/serve.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:48: if
(commandOptions.rest.isEmpty) {
On 2014/05/29 19:30:09, nweiz wrote:
> This feels weird to me. I get that it's nice to have just "pub run" do
something
> intuitive, but I really don't like that the user has to do something different
> if they want to pass any args.

Oops, I meant to delete this. You're exactly right. I was trying this out before
I went on vacation and then decided against it (for the same reason you say),
but forgot to remove this.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:56: var scriptPath =
"bin/$command.dart";
On 2014/05/29 19:30:09, nweiz wrote:
> Add a TODO to support commands in other directories.

Done.

> Also, maybe throw an error if [command] contains a path separator, since that
> behavior will change in the future.

I'm planning to implement this pretty much immediately, so I don't think it's
worth checking for it now.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:68: return
Process.start(Platform.executable, args);
On 2014/05/29 19:30:09, nweiz wrote:
> I think this should run in checked mode by default.

Done.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/05/29 19:30:09, nweiz wrote:
> File a bug against dart:io to support "exec()" on applicable platforms and add
a
> TODO to use it here.

I could be wrong, but I don't think we'd be able to use exec() here, even if
supported. We need pub to stick around so that it can continue to serve the Dart
files that the spawned VM imports.

> Manually piping the streams has several downsides, perhaps
> most notably that [stdioType] will return inaccurate results in the inner
> process, which means that programs can't safely use colorized output.

:(

Can we work on that in a later patch? I agree this is pretty rudimentary. Added
a TODO to do something better.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:78: log.error("Could not find
$scriptPath.");
On 2014/05/29 19:30:09, nweiz wrote:
> [log.fine] the (Chain-ified) stack trace as well.

Done.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:80: }, test: (error) => error is
AssetNotFoundException);
On 2014/05/29 19:30:09, nweiz wrote:
> Nit: I prefer using "if (error is! AssetNotFoundException)" in the body to
using
> [test]. It matches our style elsewhere and puts the condition closer to where
it
> would be if this were a try/catch.

Done. I used "test:" because I think it maintains a better stack trace for
errors that aren't caught. Do you think that matters?

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:230: return callback();
On 2014/05/29 19:30:09, nweiz wrote:
> Nit: This looks like it can fit on the previous line.

Done.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart (right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart:33:
pub.writeLine("second");
On 2014/05/29 19:30:09, nweiz wrote:
> Why aren't you expecting the script to print "first" and "second" here as
well?

<sigh> Still getting the last bit of seawater out of my brain.

Done.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/nonexistent_script_test.dart (right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/nonexistent_script_test.dart:11:
integration('runs a Dart application in the entrypoint package', () {
On 2014/05/29 19:30:09, nweiz wrote:
> Make the description match the filename.

Done.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
File sdk/lib/_internal/pub/test/run/runs_transformer_in_entrypoint_test.dart
(right):

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/test/r...
sdk/lib/_internal/pub/test/run/runs_transformer_in_entrypoint_test.dart:19:
integration('runs a Dart application in the entrypoint package', () {
On 2014/05/29 19:30:09, nweiz wrote:
> Make the description match the filename.

Done.

[Bob Nystrom, 2014-06-10 19:57:26]

Now that I think we're mostly in the clear for 1.5, friendly ping to get this
going again.

[nweiz, 2014-06-10 20:20:19]

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/05/30 18:17:47, Bob Nystrom wrote:
> On 2014/05/29 19:30:09, nweiz wrote:
> > File a bug against dart:io to support "exec()" on applicable platforms and
add
> a
> > TODO to use it here.
> 
> I could be wrong, but I don't think we'd be able to use exec() here, even if
> supported. We need pub to stick around so that it can continue to serve the
Dart
> files that the spawned VM imports.

There's a trick to making it work, although we'll also need fork() which I
forgot to mention. We fork the process, serve the assets from the forked child,
and call exec in the parent process.

That said, any sort of process spawning will only work with HTTP imports, not
import hooks. Why aren't we just spawning an isolate here?

> > Manually piping the streams has several downsides, perhaps
> > most notably that [stdioType] will return inaccurate results in the inner
> > process, which means that programs can't safely use colorized output.
> 
> :(
> 
> Can we work on that in a later patch? I agree this is pretty rudimentary.
Added
> a TODO to do something better.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:80: }, test: (error) => error is
AssetNotFoundException);
On 2014/05/30 18:17:47, Bob Nystrom wrote:
> On 2014/05/29 19:30:09, nweiz wrote:
> > Nit: I prefer using "if (error is! AssetNotFoundException)" in the body to
> using
> > [test]. It matches our style elsewhere and puts the condition closer to
where
> it
> > would be if this were a try/catch.
> 
> Done. I used "test:" because I think it maintains a better stack trace for
> errors that aren't caught. Do you think that matters?

[catchError] will retain the original stack trace if the original error is
re-thrown.

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

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:85: int get hashCode => name.hashCode;
This shouldn't be necessary. The default hashCode implementation supports
identity equality, which is all we need for enums. Same goes for
Verbosity.hashCode.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:159: }
Consider making the isLevelVisible function I added a method on Verbosity.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart:36:
pub.stdout.expect(matches(new RegExp("^Loading source assets")));
Rather than include this in every test, maybe we should have a pubRun wrapper
function like we do for pubServe?

Also, you can use startsWith() rather than matches() here and elsewhere.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:24: // is
started.
Explain why this is important.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:35: Future
apply(Transform transform) {
This shouldn't have a void return type.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:38: // Just
pass the primary input through.
This isn't necessary any more. Same with declaring below.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:42: Future
declareOutputs(DeclaringTransform transform) {
This should also have a void return type.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/runs_the_script_in_checked_mode_test.dart
(right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/runs_the_script_in_checked_mode_test.dart:26:
schedulePub(args: ["run", "script"],
Why doesn't this need the "loading source assets" message thing?

[Bob Nystrom, 2014-06-11 18:07:34]

Thanks!

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/06/10 20:20:18, nweiz wrote:
> On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > On 2014/05/29 19:30:09, nweiz wrote:
> > > File a bug against dart:io to support "exec()" on applicable platforms and
> add
> > a
> > > TODO to use it here.
> > 
> > I could be wrong, but I don't think we'd be able to use exec() here, even if
> > supported. We need pub to stick around so that it can continue to serve the
> Dart
> > files that the spawned VM imports.
> 
> There's a trick to making it work, although we'll also need fork() which I
> forgot to mention. We fork the process, serve the assets from the forked
child,
> and call exec in the parent process.
> 
> That said, any sort of process spawning will only work with HTTP imports, not
> import hooks. Why aren't we just spawning an isolate here?

I figured we're eventually want to support running non-Dart executables (shell
scripts, etc.) from other packages too, so spawning a process seemed more
amenable for that.

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:80: }, test: (error) => error is
AssetNotFoundException);
On 2014/06/10 20:20:18, nweiz wrote:
> On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > On 2014/05/29 19:30:09, nweiz wrote:
> > > Nit: I prefer using "if (error is! AssetNotFoundException)" in the body to
> > using
> > > [test]. It matches our style elsewhere and puts the condition closer to
> where
> > it
> > > would be if this were a try/catch.
> > 
> > Done. I used "test:" because I think it maintains a better stack trace for
> > errors that aren't caught. Do you think that matters?
> 
> [catchError] will retain the original stack trace if the original error is
> re-thrown.

TIL.

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

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:85: int get hashCode => name.hashCode;
On 2014/06/10 20:20:18, nweiz wrote:
> This shouldn't be necessary. The default hashCode implementation supports
> identity equality, which is all we need for enums. Same goes for
> Verbosity.hashCode.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/log.dart:159: }
On 2014/06/10 20:20:18, nweiz wrote:
> Consider making the isLevelVisible function I added a method on Verbosity.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/app_can_read_from_stdin_test.dart:36:
pub.stdout.expect(matches(new RegExp("^Loading source assets")));
On 2014/06/10 20:20:18, nweiz wrote:
> Rather than include this in every test, maybe we should have a pubRun wrapper
> function like we do for pubServe?
> 
> Also, you can use startsWith() rather than matches() here and elsewhere.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:24: // is
started.
On 2014/06/10 20:20:19, nweiz wrote:
> Explain why this is important.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:35: Future
apply(Transform transform) {
On 2014/06/10 20:20:19, nweiz wrote:
> This shouldn't have a void return type.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:38: // Just
pass the primary input through.
On 2014/06/10 20:20:19, nweiz wrote:
> This isn't necessary any more. Same with declaring below.

Done here.

I do have to declare the output in declareOutputs(). If I don't, the transformer
doesn't run because it's lazy and doesn't declare that it outputs the requested
asset.

This is an interesting semantic corner case. To distinguish a declaring
transformer that overwrites a primary input from one that just reads it and
produces nothing (but does not explicitly consume it), we have to explicitly
declare the pass-through asset, even though in apply() we don't have to pass it
through.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:42: Future
declareOutputs(DeclaringTransform transform) {
On 2014/06/10 20:20:19, nweiz wrote:
> This should also have a void return type.

Done.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/runs_the_script_in_checked_mode_test.dart
(right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/runs_the_script_in_checked_mode_test.dart:26:
schedulePub(args: ["run", "script"],
On 2014/06/10 20:20:19, nweiz wrote:
> Why doesn't this need the "loading source assets" message thing?

Since we just validate that stderr contains some output, the test ignores
whatever gets printed on stdout.

[nweiz, 2014-06-12 01:17:37]

lgtm

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/06/11 18:07:33, Bob Nystrom wrote:
> On 2014/06/10 20:20:18, nweiz wrote:
> > On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > > On 2014/05/29 19:30:09, nweiz wrote:
> > > > File a bug against dart:io to support "exec()" on applicable platforms
and
> > add
> > > a
> > > > TODO to use it here.
> > > 
> > > I could be wrong, but I don't think we'd be able to use exec() here, even
if
> > > supported. We need pub to stick around so that it can continue to serve
the
> > Dart
> > > files that the spawned VM imports.
> > 
> > There's a trick to making it work, although we'll also need fork() which I
> > forgot to mention. We fork the process, serve the assets from the forked
> child,
> > and call exec in the parent process.
> > 
> > That said, any sort of process spawning will only work with HTTP imports,
not
> > import hooks. Why aren't we just spawning an isolate here?
> 
> I figured we're eventually want to support running non-Dart executables (shell
> scripts, etc.) from other packages too, so spawning a process seemed more
> amenable for that.

Okay, but for Dart processes we'll definitely need to use isolates so we can
control the imports, right?

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:38: // Just
pass the primary input through.
On 2014/06/11 18:07:34, Bob Nystrom wrote:
> On 2014/06/10 20:20:19, nweiz wrote:
> > This isn't necessary any more. Same with declaring below.
> 
> Done here.
> 
> I do have to declare the output in declareOutputs(). If I don't, the
transformer
> doesn't run because it's lazy and doesn't declare that it outputs the
requested
> asset.
> 
> This is an interesting semantic corner case. To distinguish a declaring
> transformer that overwrites a primary input from one that just reads it and
> produces nothing (but does not explicitly consume it), we have to explicitly
> declare the pass-through asset, even though in apply() we don't have to pass
it
> through.

If this is true, it's a bug in barback. [declareOutputs] should default to
declaring that the primary input is passed through, just like [apply] does.
That's why [DeclaringTransform.consumePrimary] exists. Can you file an issue to
track this?

[Bob Nystrom, 2014-06-12 21:17:50]

Thanks!

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/06/12 01:17:37, nweiz wrote:
> On 2014/06/11 18:07:33, Bob Nystrom wrote:
> > On 2014/06/10 20:20:18, nweiz wrote:
> > > On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > > > On 2014/05/29 19:30:09, nweiz wrote:
> > > > > File a bug against dart:io to support "exec()" on applicable platforms
> and
> > > add
> > > > a
> > > > > TODO to use it here.
> > > > 
> > > > I could be wrong, but I don't think we'd be able to use exec() here,
even
> if
> > > > supported. We need pub to stick around so that it can continue to serve
> the
> > > Dart
> > > > files that the spawned VM imports.
> > > 
> > > There's a trick to making it work, although we'll also need fork() which I
> > > forgot to mention. We fork the process, serve the assets from the forked
> > child,
> > > and call exec in the parent process.
> > > 
> > > That said, any sort of process spawning will only work with HTTP imports,
> not
> > > import hooks. Why aren't we just spawning an isolate here?
> > 
> > I figured we're eventually want to support running non-Dart executables
(shell
> > scripts, etc.) from other packages too, so spawning a process seemed more
> > amenable for that.
> 
> Okay, but for Dart processes we'll definitely need to use isolates so we can
> control the imports, right?

Do you mean non-package absolute path imports?

The "package:" ones are handled here. Since we spawn it with an HTTP entrypoint,
all of the "package:" imports are resolved relative to that and bounce back to
pub so we can handle them.

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
File sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart (right):

https://codereview.chromium.org/301063002/diff/40001/sdk/lib/_internal/pub/te...
sdk/lib/_internal/pub/test/run/displays_transformer_logs_test.dart:38: // Just
pass the primary input through.
On 2014/06/12 01:17:37, nweiz wrote:
> On 2014/06/11 18:07:34, Bob Nystrom wrote:
> > On 2014/06/10 20:20:19, nweiz wrote:
> > > This isn't necessary any more. Same with declaring below.
> > 
> > Done here.
> > 
> > I do have to declare the output in declareOutputs(). If I don't, the
> transformer
> > doesn't run because it's lazy and doesn't declare that it outputs the
> requested
> > asset.
> > 
> > This is an interesting semantic corner case. To distinguish a declaring
> > transformer that overwrites a primary input from one that just reads it and
> > produces nothing (but does not explicitly consume it), we have to explicitly
> > declare the pass-through asset, even though in apply() we don't have to pass
> it
> > through.
> 
> If this is true, it's a bug in barback. [declareOutputs] should default to
> declaring that the primary input is passed through, just like [apply] does.
> That's why [DeclaringTransform.consumePrimary] exists. Can you file an issue
to
> track this?

Sure thing: https://code.google.com/p/dart/issues/detail?id=19408

[Bob Nystrom, 2014-06-12 21:22:37]

Committed patchset #5 manually as r37292 (presubmit successful).

[nweiz, 2014-06-12 21:22:53]

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

https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to the
spawned process.
On 2014/06/12 21:17:50, Bob Nystrom wrote:
> On 2014/06/12 01:17:37, nweiz wrote:
> > On 2014/06/11 18:07:33, Bob Nystrom wrote:
> > > On 2014/06/10 20:20:18, nweiz wrote:
> > > > On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > > > > On 2014/05/29 19:30:09, nweiz wrote:
> > > > > > File a bug against dart:io to support "exec()" on applicable
platforms
> > and
> > > > add
> > > > > a
> > > > > > TODO to use it here.
> > > > > 
> > > > > I could be wrong, but I don't think we'd be able to use exec() here,
> even
> > if
> > > > > supported. We need pub to stick around so that it can continue to
serve
> > the
> > > > Dart
> > > > > files that the spawned VM imports.
> > > > 
> > > > There's a trick to making it work, although we'll also need fork() which
I
> > > > forgot to mention. We fork the process, serve the assets from the forked
> > > child,
> > > > and call exec in the parent process.
> > > > 
> > > > That said, any sort of process spawning will only work with HTTP
imports,
> > not
> > > > import hooks. Why aren't we just spawning an isolate here?
> > > 
> > > I figured we're eventually want to support running non-Dart executables
> (shell
> > > scripts, etc.) from other packages too, so spawning a process seemed more
> > > amenable for that.
> > 
> > Okay, but for Dart processes we'll definitely need to use isolates so we can
> > control the imports, right?
> 
> Do you mean non-package absolute path imports?

No, I mean any imports that go through barback.

> The "package:" ones are handled here. Since we spawn it with an HTTP
entrypoint,
> all of the "package:" imports are resolved relative to that and bounce back to
> pub so we can handle them.

For now, yes, but HTTP imports are going away. Why design towards them?

[Bob Nystrom, 2014-06-12 21:49:47]

On 2014/06/12 21:22:53, nweiz wrote:
>
https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
> File sdk/lib/_internal/pub/lib/src/command/run.dart (right):
> 
>
https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
> sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to
the
> spawned process.
> On 2014/06/12 21:17:50, Bob Nystrom wrote:
> > On 2014/06/12 01:17:37, nweiz wrote:
> > > On 2014/06/11 18:07:33, Bob Nystrom wrote:
> > > > On 2014/06/10 20:20:18, nweiz wrote:
> > > > > On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > > > > > On 2014/05/29 19:30:09, nweiz wrote:
> > > > > > > File a bug against dart:io to support "exec()" on applicable
> platforms
> > > and
> > > > > add
> > > > > > a
> > > > > > > TODO to use it here.
> > > > > > 
> > > > > > I could be wrong, but I don't think we'd be able to use exec() here,
> > even
> > > if
> > > > > > supported. We need pub to stick around so that it can continue to
> serve
> > > the
> > > > > Dart
> > > > > > files that the spawned VM imports.
> > > > > 
> > > > > There's a trick to making it work, although we'll also need fork()
which
> I
> > > > > forgot to mention. We fork the process, serve the assets from the
forked
> > > > child,
> > > > > and call exec in the parent process.
> > > > > 
> > > > > That said, any sort of process spawning will only work with HTTP
> imports,
> > > not
> > > > > import hooks. Why aren't we just spawning an isolate here?
> > > > 
> > > > I figured we're eventually want to support running non-Dart executables
> > (shell
> > > > scripts, etc.) from other packages too, so spawning a process seemed
more
> > > > amenable for that.
> > > 
> > > Okay, but for Dart processes we'll definitely need to use isolates so we
can
> > > control the imports, right?
> > 
> > Do you mean non-package absolute path imports?
> 
> No, I mean any imports that go through barback.
> 
> > The "package:" ones are handled here. Since we spawn it with an HTTP
> entrypoint,
> > all of the "package:" imports are resolved relative to that and bounce back
to
> > pub so we can handle them.
> 
> For now, yes, but HTTP imports are going away. 

You get your $20 yet? :)

> Why design towards them?

There's not much code based on them here, it was just the simplest
implementation that worked. I don't think it would be too hard to switch to an
isolate later.

[nweiz, 2014-06-12 22:29:32]

On 2014/06/12 21:49:47, Bob Nystrom wrote:
> On 2014/06/12 21:22:53, nweiz wrote:
> >
>
https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
> > File sdk/lib/_internal/pub/lib/src/command/run.dart (right):
> > 
> >
>
https://codereview.chromium.org/301063002/diff/1/sdk/lib/_internal/pub/lib/sr...
> > sdk/lib/_internal/pub/lib/src/command/run.dart:70: // Wire up pub's pipes to
> the
> > spawned process.
> > On 2014/06/12 21:17:50, Bob Nystrom wrote:
> > > On 2014/06/12 01:17:37, nweiz wrote:
> > > > On 2014/06/11 18:07:33, Bob Nystrom wrote:
> > > > > On 2014/06/10 20:20:18, nweiz wrote:
> > > > > > On 2014/05/30 18:17:47, Bob Nystrom wrote:
> > > > > > > On 2014/05/29 19:30:09, nweiz wrote:
> > > > > > > > File a bug against dart:io to support "exec()" on applicable
> > platforms
> > > > and
> > > > > > add
> > > > > > > a
> > > > > > > > TODO to use it here.
> > > > > > > 
> > > > > > > I could be wrong, but I don't think we'd be able to use exec()
here,
> > > even
> > > > if
> > > > > > > supported. We need pub to stick around so that it can continue to
> > serve
> > > > the
> > > > > > Dart
> > > > > > > files that the spawned VM imports.
> > > > > > 
> > > > > > There's a trick to making it work, although we'll also need fork()
> which
> > I
> > > > > > forgot to mention. We fork the process, serve the assets from the
> forked
> > > > > child,
> > > > > > and call exec in the parent process.
> > > > > > 
> > > > > > That said, any sort of process spawning will only work with HTTP
> > imports,
> > > > not
> > > > > > import hooks. Why aren't we just spawning an isolate here?
> > > > > 
> > > > > I figured we're eventually want to support running non-Dart
executables
> > > (shell
> > > > > scripts, etc.) from other packages too, so spawning a process seemed
> more
> > > > > amenable for that.
> > > > 
> > > > Okay, but for Dart processes we'll definitely need to use isolates so we
> can
> > > > control the imports, right?
> > > 
> > > Do you mean non-package absolute path imports?
> > 
> > No, I mean any imports that go through barback.
> > 
> > > The "package:" ones are handled here. Since we spawn it with an HTTP
> > entrypoint,
> > > all of the "package:" imports are resolved relative to that and bounce
back
> to
> > > pub so we can handle them.
> > 
> > For now, yes, but HTTP imports are going away. 
> 
> You get your $20 yet? :)
> 
> > Why design towards them?
> 
> There's not much code based on them here, it was just the simplest
> implementation that worked. I don't think it would be too hard to switch to an
> isolate later.

I'm mostly worried about the user-visible differences between running in the
top-level process vs running in a nested process, such as the stdio stuff I
mentioned above. I don't want users to get the impression that they can't use
[stdioType] in a "pub run" process.