Skip to content

Add the ability to spawn an isolate in Dart 2 preview mode #32253

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
nex3 opened this issue Feb 20, 2018 · 13 comments
Closed

Add the ability to spawn an isolate in Dart 2 preview mode #32253

nex3 opened this issue Feb 20, 2018 · 13 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. customer-dart-2 customer-pub library-isolate

Comments

@nex3
Copy link
Member

nex3 commented Feb 20, 2018

Pub users want to be able to run their executables via pub run using Dart 2 preview mode (see dart-lang/pub#1807 and flutter/flutter#14728 (comment)). Because pub run runs executables in isolates, this means it'll need to be able to spawn an isolate that uses Dart 2 preview mode.

I suggest adding a bool previewDart2 flag to Isolate.spawnUri() that will become a deprecated no-op once Dart 2 releases.

@nex3 nex3 added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-isolate customer-dart-2 customer-pub labels Feb 20, 2018
@lrhn
Copy link
Member

lrhn commented Feb 21, 2018

I'd much rather have the Dart version embedded in the dill file that the VM uses to spawn the isolate.
I see no advantage in supporting running actual Dart 2 code from source since that will not work anyway (the VM doesn't do type inference, and Dart 2 without type inference doesn't really work).

Alternatively, make pub itself run with the flag set. Perhaps make the pub script accept the --preview-dart2 flag and run itself on a VM with that flag. The pub code should still work, and anything it spawns will inherit the flag. I assume that there would need to be a flag on pub run anyway..

@vsmenon
Copy link
Member

vsmenon commented Feb 21, 2018

@kevmoo @devoncarew How important is this for general travis testing with Dart 2?

Some options:

  1. Add the isolate support as requested. Not sure it's actually possible for the VM to have some isolates on the CFE and some not (@a-siva ).

  2. Get pub itself running on Dart 2 as Lasse suggested. Not clear how much work it'll be to run it and its dependences on Dart 2. We haven't gotten most of our own tools running in 2 yet.

  3. Rework pub to not use isolate (and, e.g., start a new Dart process instead). @nex3 - is that feasible?

@nex3
Copy link
Member Author

nex3 commented Feb 21, 2018

I'd much rather have the Dart version embedded in the dill file that the VM uses to spawn the isolate.
I see no advantage in supporting running actual Dart 2 code from source since that will not work anyway (the VM doesn't do type inference, and Dart 2 without type inference doesn't really work).

Hold on a sec--if we don't plan to support running Dart 2 code from source, that's news to me. It also has major implications for all sorts of tooling. My understanding was that the VM would embed the common front end to do the initial type checking. Isn't that how --preview-dart-2 works today?

Alternatively, make pub itself run with the flag set. Perhaps make the pub script accept the --preview-dart2 flag and run itself on a VM with that flag. The pub code should still work, and anything it spawns will inherit the flag. I assume that there would need to be a flag on pub run anyway..

#32188 is tracking this, but there are a lot of complexities involved. In addition to getting the transitive closure of pub (which includes the analyzer and dart2js) Dart-2-clean, we'd need to build both Dart 1 and Dart 2 snapshots of pub, bundle them with the SDK, and have pub's wrapper scripts parse arguments to figure out which one to invoke. It's not impossible, but it would be a lot of work.

Rework pub to not use isolate (and, e.g., start a new Dart process instead). @nex3 - is that feasible?

We used to do this, and it caused all kinds of problems around performance, signal-handling, and standard IO piping. I don't want to regress that behavior, and I'm not keen on adding an alternate code path that has more behavioral differences than just the Dart 2 semantics.

@a-siva
Copy link
Contributor

a-siva commented Feb 21, 2018

@nex3 - The VM supports running Dart 2 code from source we don't plan on getting rid of that.

@vsmenon - about the ability for the VM to run some isolates in strong mode and others not - we don't currently have support for this when running from source but it should be possible to do this (currently we start the CFE isolate eagerly under the --preview-dart-2 flag but if we made that lazy then this should be possible).

@a-siva
Copy link
Contributor

a-siva commented Mar 9, 2018

@rmacnak-google (see offline email about a possible way to get this working quickly)

@tvolkert
Copy link
Contributor

tvolkert commented Apr 12, 2018

re: #32253 (comment)

@kevmoo @devoncarew How important is this for general travis testing with Dart 2?

IMO, very important. I just rolled a new version of package:redux into Google and discovered that it didn't work in some cases under Dart 2, but there was no way to write tests that showed the failure and have those failures surface in their Travis setup.

If we're asking our ecosystem to try to be fully Dart-2 compliant by I/O, we need to give them the tools to do so...

/cc @eseidelGoogle

(I found my way here from dart-lang/pub#1807)

@tvolkert
Copy link
Contributor

Discussing with @a-siva it sounds like the right solution to this is just to make pub Dart 2 compliant, which would obviate the need for this feature request to be resolved -- because the main pub isolate could be started in Dart 2.

@kevmoo
Copy link
Member

kevmoo commented Apr 12, 2018

the right solution to this is just to make pub Dart 2 compliant

Landing dart-lang/pub#1857 first will make this a lot easier, since the surface area of pub will be reduced substantially. ETA – next few days.

@eseidelGoogle
Copy link

Please confirm my understanding:

  1. It is not currently possible to use/run a package:test test which use dart2 (or tests for dart2 specific issues), correct?
  2. It is possible to use/run a flutter test test in dart 2 (that's the only way) due to flutter_test's different test backend.

@natebosch
Copy link
Member

It is not currently possible to use/run a package:test test which use dart2 (or tests for dart2 specific issues), correct?

It is not currently possible to run a VM test with dart 2 semantics using pub run test

It is possible to run a DDC test with dart 2 semantics.

I think it should be possible to run a VM test with dart 2 semantics using a hacky workaround of invoking the package:test entrypoint directly (have you find it first) instead of going through pub run.
There may be dart 2 semantics issues in the package:test code which would also prevent this.

@natebosch
Copy link
Member

Just tried this. package:test is not Dart 2 clean. There are a few bugs I can fix, but then there is also one in package:front_end getting pull in through analyzer...

@natebosch
Copy link
Member

natebosch commented Apr 12, 2018

Filed an issue for the bug in front_end: #32867

I can't confirm with certainty that fixing this will unblock testing though.

@natebosch
Copy link
Member

Closing since I don't think we're going to support running Isolates in a different mode from the rest of the VM.

For now the solution is DART_VM_OPTIONS=--preview-dart-2 and long term that will become the only mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. customer-dart-2 customer-pub library-isolate
Projects
None yet
Development

No branches or pull requests

8 participants