Skip to content

"pub run" should spawn an isolate rather than starting a subprocess #1204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DartBot opened this issue Jun 5, 2015 · 19 comments
Closed

"pub run" should spawn an isolate rather than starting a subprocess #1204

DartBot opened this issue Jun 5, 2015 · 19 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="96" height="96"hspace="10"> Issue by nex3
Originally opened as dart-lang/sdk#21821


Currently "pub run" starts a subprocess to run its code. It should instead spawn an isolate to avoid VM startup time and provide accurate information for the stdioType getter.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


Added Pub-Run label.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


This is blocking dart-lang/test#19.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


In the meantime could pub run temporarily set an environment variable in the sub-process for us to check the stdioType e.g. PUB_RUN_STDIOTYPE?

This is also blocking https://github.com/google/stagehand/issues/147

@DartBot DartBot added type-enhancement A request for a change that isn't a bug Pub-Run labels Jun 5, 2015
@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


This would fix stdioType, but wouldn't it break Platform.script?

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


Platform.script is already not what people expect under "pub run". Ideally there would be a way to spawn an isolate with a custom value for Platform.script, but that feature request is out-of-scope for this issue.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


I think Platform.script currently references the pub cache location of the script, no? I only need basenameWithoutExtension(Platform.script), so that has been sufficient for my needs, and I'm not sure what a better value would be. But I would no longer have that if the isolate retains the Platform.script of pub itself, rather than of the spawned script.

(BTW my use case is for unscripted to determine the command name for help text output.)

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/444270?v=3" align="left" width="48" height="48"hspace="10"> Comment by seaneagan


See also issue #1234, which would be an even better solution for my use case.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


This is also blocking dart-lang/test#86.

@DartBot
Copy link
Author

DartBot commented Jun 5, 2015

This comment was originally written by [email protected]


Case where this is a real problem:
"stdout.terminalColumns" throws when file is run with "pub run" rather than "dart"

@nex3
Copy link
Member

nex3 commented Aug 5, 2015

This is blocked on dart-lang/sdk#23951.

@joseluis
Copy link

joseluis commented Mar 8, 2016

I believe dart-lang/sdk#23951 is no longer blocking the issue, so I want to give it a bump. Since it's still not possible to use pub to run Dart console apps that makes advanced use of the terminal.

@nex3
Copy link
Member

nex3 commented Mar 8, 2016

I'm also eager to do this. At this point, it's just a question of having the spare time.

@alexander-doroshko
Copy link

IDE users keep asking for debugging capabilities of tests (tracked as dart-lang/test#50) and this feature request seems to be the most perfect solution. I guess 'blocked' label is obsolete here. Any ETA? Thanks for taking a look!

@nex3 nex3 removed the status-blocked Blocked from making progress by another (referenced) issue label May 10, 2016
@nex3
Copy link
Member

nex3 commented May 10, 2016

@alexander-doroshko dart-lang/test#50 is actually blocked on dart-lang/sdk#23320, not on this.

@alexander-doroshko
Copy link

alexander-doroshko commented May 10, 2016

@nex3 #23320 is closed as Not Planned. This request is another way to be able to enable Observatory for the VM that runs tests. We have control on VM options that runs pub (like dart -observe pub.dart.snapshot), so if pub runs the requested app in a separate isolate (instead of a separate Dart VM as of now) we are fine with debugging.

@nex3
Copy link
Member

nex3 commented May 10, 2016

dart-lang/sdk#23320 will need to be fixed in order to enable VM debugging in test; there's not really any way around that. test needs to be able to enable the observatory based on its own logic, it needs to allocate an ephemeral port, and it needs to know that ephemeral port so it can establish a connection to it. If you believe that VM debugging support in test is important (and I agree!), talk to @kasperl about prioritizing the VM issue blocking it.

@alexander-doroshko
Copy link

alexander-doroshko commented May 10, 2016

We seem to be talking about a bit different things.
Imagine that this request is fixed and pub run doesn't start a separate Dart VM, but just spawns a new isolate where the requested app is executed. In this case IDE will start tests like this:

[Dart SDK]/bin/dart --pause_isolates_on_start --enable-vm-service:50412 [Dart SDK]/bin/snapshots/pub.dart.snapshot run test:test -r json

After that IDE connects to the Observatory on 50412 port and debugging in the IDE starts. That's the way that is already used in the IDE for standard Dart VM apps debugging.

@nex3
Copy link
Member

nex3 commented May 10, 2016

The strategy you describe would probably work okay, but it isn't really related to dart-lang/test#50. In general, test will only support its built-in flags as official means of debugging, which means that your strategy would be unsupported and subject to unexpected breakage.

@alexander-doroshko
Copy link

Ok, let's keep dart-lang/test#50 aside. Indeed, I'm not asking to do anything with the test package itself. And this request is about pub run, not about test at all.

Now you see, all I need is a control over VM options that are used by the VM that runs the app if the app is started via pub run.

Being able to debug tests from the IDE will be a great side effect of this fix. I'm going to use documented and thoroughly tested Dart VM options, so I do not see any risk of unexpected breakage, even if some day new options and alternative way of debugging appear in test package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants