Remove ProcessOptions and make the options named arguments.

Remove ProcessOptions and make the options named arguments.

This also removed Process.runShell and adds a new flag: runInShell.

BUG=
R=kustermann@google.com, nweiz@google.com, sgjesse@google.com

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

[nweiz, 2013-05-23 18:55:10]

One small change, otherwise the scheduled_test changes LGTM.

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:74: /// [Process.start].
[description] is a string
Please re-justify the whole paragraph. The following one as well.

[Søren Gjesse, 2013-05-24 06:59:24]

LGTM

Great change!

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:75: * The [workingDirectory] can be changed to specify
from which directory the
Change

The [workingDirectory] can be changed to specify from which directory the
process is started

To something like

Use [workingDirectory] to set the working directory for the process

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:82: * ASCII environment variables are supported and
errors are likely to occur if
ASCII -> US-ASCII

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:106: * The [workingDirectory] can be changed to specify
from which directory the
Same change as above.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:113: * ASCII environment variables are supported and
errors are likely to occur if
Ditto.

[Søren Gjesse, 2013-05-24 08:40:54]

Remember to send out a breaking change mail when you land this. see
https://code.google.com/p/dart/wiki/HowToWriteABreakingChangeEmail.

On 2013/05/24 06:59:24, Søren Gjesse wrote:
> LGTM
> 
> Great change!
> 
> https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart
> File sdk/lib/io/process.dart (right):
> 
>
https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
> sdk/lib/io/process.dart:75: * The [workingDirectory] can be changed to specify
> from which directory the
> Change
> 
> The [workingDirectory] can be changed to specify from which directory the
> process is started
> 
> To something like
> 
> Use [workingDirectory] to set the working directory for the process
> 
>
https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
> sdk/lib/io/process.dart:82: * ASCII environment variables are supported and
> errors are likely to occur if
> ASCII -> US-ASCII
> 
>
https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
> sdk/lib/io/process.dart:106: * The [workingDirectory] can be changed to
specify
> from which directory the
> Same change as above.
> 
>
https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
> sdk/lib/io/process.dart:113: * ASCII environment variables are supported and
> errors are likely to occur if
> Ditto.

[kustermann, 2013-05-24 08:44:30]

lgtm

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:85: {workingDirectory,
environment, String description,
I wouldn't mind if we make this 'cwd'. But I guess our "don't abbreviate" policy
doesn't allow it?

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:145: new Future.sync(() =>
environment)
Why do we have 'new Future.sync(() => executable)' ...?

[floitsch, 2013-05-24 09:32:20]

DBC.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:83: * an environment variables with code-points outside
the ASCII range is passed
an environment variable (-s) contains runes outside the US-ASCII range.

(if you prefer you can stick to "code points".)

[Anders Johnsen, 2013-05-24 11:03:54]

Committed patchset #2 manually as r23131 (presubmit successful).

[Anders Johnsen, 2013-05-24 11:04:15]

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:74: /// [Process.start].
[description] is a string
On 2013/05/23 18:55:11, nweiz wrote:
> Please re-justify the whole paragraph. The following one as well.

Done.

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:85: {workingDirectory,
environment, String description,
On 2013/05/24 08:44:30, kustermann wrote:
> I wouldn't mind if we make this 'cwd'. But I guess our "don't abbreviate"
policy
> doesn't allow it?

Yep. :)

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:145: new Future.sync(() =>
environment)
On 2013/05/24 08:44:30, kustermann wrote:
> Why do we have 'new Future.sync(() => executable)' ...? 

Since the executable can be wrapped in a Future. See comment above.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart
File sdk/lib/io/process.dart (right):

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:75: * The [workingDirectory] can be changed to specify
from which directory the
On 2013/05/24 06:59:24, Søren Gjesse wrote:
> Change
> 
> The [workingDirectory] can be changed to specify from which directory the
> process is started
> 
> To something like
> 
> Use [workingDirectory] to set the working directory for the process

Done.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:82: * ASCII environment variables are supported and
errors are likely to occur if
On 2013/05/24 06:59:24, Søren Gjesse wrote:
> ASCII -> US-ASCII

Done.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:83: * an environment variables with code-points outside
the ASCII range is passed
On 2013/05/24 09:32:20, floitsch wrote:
> an environment variable (-s) contains runes outside the US-ASCII range.
> 
> (if you prefer you can stick to "code points".)

Done.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:106: * The [workingDirectory] can be changed to specify
from which directory the
On 2013/05/24 06:59:24, Søren Gjesse wrote:
> Same change as above.

Done.

https://codereview.chromium.org/15883003/diff/1/sdk/lib/io/process.dart#newco...
sdk/lib/io/process.dart:113: * ASCII environment variables are supported and
errors are likely to occur if
On 2013/05/24 06:59:24, Søren Gjesse wrote:
> Ditto.

Done.

[Yonggang Luo, 2013-05-24 18:55:59]

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:156: // TODO(nweiz): enable this
when issue 9020 is fixed.
https://code.google.com/p/dart/issues/detail?id=9020
is fixed...

[nweiz, 2013-05-24 20:15:50]

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/1/pkg/scheduled_test/lib/schedu...
pkg/scheduled_test/lib/scheduled_process.dart:156: // TODO(nweiz): enable this
when issue 9020 is fixed.
On 2013/05/24 18:55:59, Yonggang Luo wrote:
> https://code.google.com/p/dart/issues/detail?id=9020
> is fixed...

Nice catch, I'll enable this in a separate CL.

[Bill Hesse, 2013-07-22 15:36:29]

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
File tests/standalone/io/process_shell_test.dart (right):

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
tests/standalone/io/process_shell_test.dart:45: void testBadRunShell() {
This change to testBadRunShell seems totally broken, and I'm not sure what it is
even supposed to do.  PTAL.

[Anders Johnsen, 2013-07-22 15:44:21]

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
File tests/standalone/io/process_shell_test.dart (right):

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
tests/standalone/io/process_shell_test.dart:45: void testBadRunShell() {
On 2013/07/22 15:36:29, Bill Hesse wrote:
> This change to testBadRunShell seems totally broken, and I'm not sure what it
is
> even supposed to do.  PTAL.

It's supposed to test that you can't run the program '"' and '$HOME'. Nothing
fancy, just to make sure that the escaping goes through and that e.g. it's not
resolve to the empty string, that will complete with exitCode==0 on some
platforms.

[Bill Hesse, 2013-07-22 15:50:21]

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
File tests/standalone/io/process_shell_test.dart (right):

https://codereview.chromium.org/15883003/diff/10001/tests/standalone/io/proce...
tests/standalone/io/process_shell_test.dart:45: void testBadRunShell() {
On 2013/07/22 15:44:22, Anders Johnsen wrote:
> On 2013/07/22 15:36:29, Bill Hesse wrote:
> > This change to testBadRunShell seems totally broken, and I'm not sure what
it
> is
> > even supposed to do.  PTAL.
> 
> It's supposed to test that you can't run the program '"' and '$HOME'. Nothing
> fancy, just to make sure that the escaping goes through and that e.g. it's not
> resolve to the empty string, that will complete with exitCode==0 on some
> platforms.


OK, so it has nothing to do with the testShell() function it replaces.  It still
is dirty code, with dead variables, unused arguments, etc.  Is it really what
was intended?

I'm removing the path variable as part of removing Path, since it is dead code.

[nweiz, 2013-07-22 20:15:10]

https://codereview.chromium.org/15883003/diff/10001/pkg/scheduled_test/lib/sc...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/10001/pkg/scheduled_test/lib/sc...
pkg/scheduled_test/lib/scheduled_process.dart:127: environment) {
These arguments shouldn't be aligned like this; they should just wrap when a
given line runs out of space. See for example [ScheduledProcess.start].

https://codereview.chromium.org/15883003/diff/10001/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/15883003/diff/10001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/test_pub.dart:374: var environment = new
Map.from(Platform.environment);
Since 9294 is fixed, this should just be initialized to a map literal.

[Søren Gjesse, 2013-07-23 13:03:59]

https://codereview.chromium.org/15883003/diff/10001/pkg/scheduled_test/lib/sc...
File pkg/scheduled_test/lib/scheduled_process.dart (right):

https://codereview.chromium.org/15883003/diff/10001/pkg/scheduled_test/lib/sc...
pkg/scheduled_test/lib/scheduled_process.dart:127: environment) {
On 2013/07/22 20:15:11, nweiz wrote:
> These arguments shouldn't be aligned like this; they should just wrap when a
> given line runs out of space. See for example [ScheduledProcess.start].

In the C++ style guide (which is the basis for the Dart style guide) both types
are mentioned, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla....
In dart:io at least we have been having all on one line or separate lines for
each argument if they could not fit on one line.