Skip to content

pub doesn't support --enable-experiment=non-nullable #41355

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
mit-mit opened this issue Apr 6, 2020 · 19 comments
Closed

pub doesn't support --enable-experiment=non-nullable #41355

mit-mit opened this issue Apr 6, 2020 · 19 comments
Assignees
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). NNBD Issues related to NNBD Release
Milestone

Comments

@mit-mit
Copy link
Member

mit-mit commented Apr 6, 2020

bin mit$ ./pub run --enable-experiment=non-nullable ~/tmp/app01/bin/main.dart
Could not find an option named "enable-experiment".
@mit-mit mit-mit added this to the D29 Release milestone Apr 6, 2020
@mit-mit mit-mit added the NNBD Issues related to NNBD Release label Apr 6, 2020
@a-siva a-siva added the area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). label Apr 7, 2020
@a-siva
Copy link
Contributor

a-siva commented Apr 7, 2020

Moved this issue to the pub repo at dart-lang/pub#2436

@a-siva a-siva closed this as completed Apr 7, 2020
@kevmoo
Copy link
Member

kevmoo commented Apr 9, 2020

The way to implement this will be in the SDK wrapper scripts – not in the pub repo.

@mit-mit
Copy link
Member Author

mit-mit commented Apr 24, 2020

Copying over comment from @natebosch in #41403:

The idea here is that we'd support a minimal amount of arg parsing in the script so that pub --enable-experiment=non-nullable run gets turned into VM argument correctly.

@mit-mit
Copy link
Member Author

mit-mit commented Apr 24, 2020

@kevmoo @natebosch do we have any existing scripts that do something like this?

@natebosch
Copy link
Member

do we have any existing scripts that do something like this?

Not that I'm aware of. It would likely involve some manual argument iteration and string prefix comparison for --enable-experiment=. I might be able to help with the bash script, It would take me longer to figure it out in the windows one.

@jonasfj
Copy link
Member

jonasfj commented May 13, 2020

In bash we can do this with:

if [[ "$1" == '--enable-experiment='* ]]; then
  VM_OPTIONS+=("$1")
  shift;
fi

But for .bat files I can't find a good solution this question on stackoverflow links two similar questions, all 3 have multiple solutions, but none of them can handle arguments with quotes, spaces and equal signs... All things that could likely occur in arguments passed through pub run.

note. VM options can already be passed with:
DART_VM_OPTIONS='--enable-experiment=non-nullable' pub run ...

@jonasfj
Copy link
Member

jonasfj commented May 13, 2020

@sigurdm, is there any reason we use isolates for pub run if we were to use sub-processes we could handle the --enable-experiment flag in pub, and it would be easier to embed pub in dartdev.

Also, I can't help but wonder if --enable-experiment won't cause problems with our snapshotting, if you were to switch it on/off, just thinking someone might do so with pub run test.

@natebosch
Copy link
Member

none of them can handle arguments with quotes, spaces and equal signs

The VM only handles the argument passed with an equals sign. We don't have to worry about any strangeness around spaces, which means we can also probably avoid any concerns about quoting.

@natebosch
Copy link
Member

I can't help but wonder if --enable-experiment won't cause problems with our snapshotting, if you were to switch it on/off, just thinking someone might do so with pub run test.

That's a good point, I don't know how snapshots will work with this.

@jonasfj
Copy link
Member

jonasfj commented May 13, 2020

We don't have to worry about any strangeness around spaces, which means we can also probably avoid any concerns about quoting.

I got the impression that the .bat solutions would be buggy if any of the arguments had such things. To be fair though, I don't know that our current solution can handle these corner cases.
Since it's probably rather rare that windows users pass complicated arguments through pub.

In any case, the snapshot concerns gives me reason to pause, and think if perhaps it's better if pub run creates a subprocess instead of an isolate.

@sigurdm
Copy link
Contributor

sigurdm commented May 14, 2020

@sigurdm, is there any reason we use isolates for pub run if we were to use sub-processes we could handle the --enable-experiment flag in pub, and it would be easier to embed pub in dartdev.

I don't know of the reason.
My guess would be that an isolate starts slightly faster, and consumes less memory because it can share the VM. Not sure how significant that is though.

Also, I can't help but wonder if --enable-experiment won't cause problems with our snapshotting, if you were to switch it on/off, just thinking someone might do so with pub run test.

One option would be to completely drop precompilation (and ignore existing snapshots) if --enable-experiment. That would make pub run slower with the experiment, but ensure consistency.

Otherwise we would have to save information about the configuration when the snapshot was created, so we can recreate it the configuration changed. That might be handy for other configuration than just --enable-experiment.

@natebosch
Copy link
Member

My guess would be that an isolate starts slightly faster, and consumes less memory because it can share the VM. Not sure how significant that is though.

I think it used to be fairly significant. I haven't measured in a while.

One option would be to completely drop precompilation (and ignore existing snapshots) if --enable-experiment. That would make pub run slower with the experiment, but ensure consistency.

This seems like a good idea. I think we'd need a way to ask within the runtime whether any experiments are enabled.

@franklinyow
Copy link
Contributor

@jonasfj This issue is in the critical path of tech preview. I like to get a ETA on this so I can build a timeline. Thanks!

@jonasfj
Copy link
Member

jonasfj commented May 14, 2020

Hmm, looking at the dartdev run it uses a subprocess. Hence, that must be good enough for pub run too.

Running pub with experiments is undesirable anyways. As it might break/affect pub which is not what you want when you do pub run --enable-experiment=...

@franklinyow, hmm, we have a few things to refactor to get this going. But a week or maybe two (given all the holidays coming up).

@franklinyow
Copy link
Contributor

Any update on this?

@mit-mit
Copy link
Member Author

mit-mit commented May 28, 2020

@sigurdm @jonasfj is dart-lang/pub#2493 still the active PR for this?

@sigurdm
Copy link
Contributor

sigurdm commented May 28, 2020

Yes

@rakudrama
Copy link
Member

How can we enable the experiment on travis?
AFAIK the travis task delegates to pub run test.
I want to be sure package:fixnum is tested both opt-in and opt-out.

This is an example on travis where I tried adding --enable-experiment=non-nullable to a travis test.
dart-archive/fixnum@3d2eb5b
It fails like this:
https://travis-ci.org/github/dart-lang/fixnum/jobs/694414777

/cc @kevmoo

@kevmoo
Copy link
Member

kevmoo commented Jun 3, 2020

Fixed in 64b761b

@kevmoo kevmoo closed this as completed Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

8 participants