Use subprocess for "pub run" and allow vm options

[jonasfj]

We should allow vm options, I think using a subprocess is a fine solution. Let's try it :)

[sigurdm]

Should we do the vm-options something like this:?

pub run --vm-options="--enable-experiment" test

[jakemac53]

There is no need imo for --vm-options, just use the position of the args instead?

This is already done for the --enable-asserts option (which is actually a vm option already right?).

So the usage would be pub run [vm-options...] <command> [command-options...]

[sigurdm]

There is no need imo for --vm-options, just use the position of the args instead?

Hmm --- not a bad idea.
I just worry it will be hard to follow what arguments goes where (eg. --verbose should still go to pub). Also the VM arguments are somewhat 'raw' compared to what pub usually provides, and we might want to present only the "common subset" for passing directly.

Snapshotting is also an issue. If we pass arbitrary vm arguments we probably cannot rely on a snapshot and should always run from source, but for '--enable-asserts' and some other curated subset we know that we can use a snapshot that was compiled without these flags.

[jakemac53]

I just worry it will be hard to follow what arguments goes where (eg. --verbose should still go to pub).

This is already exactly how --enable-asserts works though - it has to come before the binary name you want to run. The --vm-options flag should similarly work the same way otherwise no pub binary can have an option with that name.

Also the VM arguments are somewhat 'raw' compared to what pub usually provides, and we might want to present only the "common subset" for passing directly.

Imo its more confusing if some flags work and some don't. A simple pass through of the args is very easy to understand. Simplicity ftw :).

Snapshotting is also an issue. If we pass arbitrary vm arguments we probably cannot rely on a snapshot and should always run from source, but for '--enable-asserts' and some other curated subset we know that we can use a snapshot that was compiled without these flags.

You still have access to the args before you pass them on, and could still made an educated decision to run from snapshot when a known compatible set of flags is passed.

[jonasfj]

pub run [vm-options...] <command> [command-options...]
I like this... we can allow an optional -- as separator..

pub run [vm-options...] [--] <command> [command-options...]

The only downside I see is that when pub run --v2 --some-arg or dartdev run --some-arg and we start the default command from the current package, then how do we know if --some-arg is a VM option or a command option.

The logical thing is to interpret it as a VM option. But that makes pub run --v2 --custom-arg not work, and users will have to type pub run --v2 -- --custom-arg. This is not a big issue for pub run as I don't anticipate users will be using --v2, but replace pub run --v2 with dartdev run and it's less than ideal..

[jakemac53]

I had no idea there was even a default... is it bin/main.dart? I think its fine to just not support any vm options at all in that case - act as if all the args were passed to the program. You can easily work around that by explicitly passing the path.

How does this work with --enable-asserts today?

[jonasfj]

I had no idea there was even a default...

There isn't one yet :D

But as part of dartdev run we've agreed to make one which runs bin/<packageName>.dart- Because today pub run <packageName> runs bin/<packageName>.dart, so it seemed consistent to have dartdev run imply dartdev run <rootPackageName>.

[jonasfj]

How does this work with --enable-asserts today?

I think we explicitly recognize --enable-asserts today, and we don't have a default command yet.

Hmm, we could also pass VM options before run, so pub [vm-options] run <command> ....
Edit: Not sure I'm fan.. Maybe it's just better that it's hard to pass options to the default command.

[sigurdm]

I am more or less convinced :) . I'll make all args before the command pass through to the vm. And for v2 we can say that if there is no command all args go to the program, but -- stands in for the command. Thanks!

[jakemac53]

But as part of dartdev run we've agreed to make one which runs bin/<packageName>.dart- Because today pub run <packageName> runs bin/<packageName>.dart, so it seemed consistent to have dartdev run imply dartdev run <rootPackageName>.

Running pub run results in Must specify and executable to run. This new feature is not mirroring any existing functionality, and I don't think it provides much value personally. It is a bit to magic for my personal taste, and its just yet another way to do the same thing. It also introduces ambiguity, for instance I would expect it to run bin/main.dart so I am already confused by it.

[franklinyow]

@kevmoo Is this necessary for TP1?

[mit-mit]

I think this really boils down to the same as dart-lang/sdk#41355 ?

[kevmoo]

Agreed!

…

On Fri, May 29, 2020 at 12:27 AM Michael Thomsen ***@***.***> wrote: I think this really boils down to the same as dart-lang/sdk#41355 <dart-lang/sdk#41355> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2492 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCWHS3LKD6ML6IWS7UTRT5PX5ANCNFSM4NBCWKPQ> .

[kevmoo]

Could we use a subprocess ONLY when using VM options? I worry about startup cost here...

…

On Fri, May 29, 2020 at 3:35 PM Kevin Moore ***@***.***> wrote: Agreed! On Fri, May 29, 2020 at 12:27 AM Michael Thomsen ***@***.***> wrote: > I think this really boils down to the same as dart-lang/sdk#41355 > <dart-lang/sdk#41355> ? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#2492 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAEFCWHS3LKD6ML6IWS7UTRT5PX5ANCNFSM4NBCWKPQ> > . >

[sigurdm]

Could we use a subprocess ONLY when using VM options? I worry about startup cost here...

Yes - we landed on doing that anyway (for now at least) mainly because of not wanting to regress on signal handling. But startup time is also a factor - we however should measure this before assuming it is a problem.

[sigurdm]

I tried informally running pub run hello_world.dart (pub was already compiled to a snapshot, hello_world.dart was in the main package, so it would not get compiled to snapshot).

Using an Isolate: ~1100 ms. (usr time)
Using a subprocess: ~750 ms. (usr time)

So surprisingly, starting the dart program using a subprocess seems faster. Not sure what to make of that - maybe I'm measuring the wrong thing.

[sigurdm]

@mkustermann does it sound surprising to you that Isolate.spawnUri('program.dart') is slower than Process.start(Platform.resolvedExecutable, ['program.dart'])?

[mkustermann]

@sigurdm Yes, it's surprising. Possibly related to the auto-NNBD detection. Maybe Isolate.spawnUri causes 2 compilations of the program to kernel, once with NNBD once without or something like this. (Just speculation)

@a-siva Do you know more about how this was implemented?

[a-siva]

Yes it is surprising, I will investigate this tomorrow, did not have time to get to this today. Do you have the sample code you were using to measure the time. I wanted to know what kind of vm_options you were using.
I would have expected the non-nullability auto detect path to be the same for both scenarios unless different vm_options were used for the two scenarios.

[a-siva]

Did you figure out what the difference was? I noticed the issue is now closed.

[sigurdm]

This issue was about changing pub. I opened a new issue about the vm.