Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

jbhensley · 2023-05-16T18:52:54Z

Add ProcessStartInfo(string fileName, IEnumerable<string> arguments). Fixes #66450.

…uments

dotnet-issue-labeler · 2023-05-16T18:53:01Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-05-16T18:53:06Z

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

Add ProcessStartInfo(string fileName, IEnumerable<string> arguments) as per #66450.

Author:	jbhensley
Assignees:	-
Labels:	`area-System.Diagnostics.Process`, `new-api-needs-documentation`
Milestone:	-

danmoseley · 2023-05-16T19:20:17Z

@jbhensley If merging this should close the issue, you can put "fixes #66450" on a line in your top comment.

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

stephentoub · 2023-05-16T21:03:30Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs

+
+            foreach (string argument in arguments)
+            {
+                ArgumentList.Add(argument);
+            }


Instead of adding each argument individually, this could be:

Suggested change

foreach (string argument in arguments)

{

ArgumentList.Add(argument);

}

if (arguments is not null)

{

_argumentList = new Collection<string>(new List<string>(arguments));

}

and let List<T>'s ctor handle it however makes sense.

adamsitnik

Thank you for your contribution @jbhensley !

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs

adamsitnik · 2023-05-17T07:54:53Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs

+        {
+            _fileName = fileName;
+
+            if(arguments != null)


Why should we allow for nulls in the new API?

Since the argument is not marked as nullable, the method should throw for nulls.

I can move these checks into the new ProcessStartInfo ctor

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs

Lines 1328 to 1331 in af1171b

public static Process Start(string fileName, IEnumerable<string> arguments)

{

ArgumentNullException.ThrowIfNull(fileName);

ArgumentNullException.ThrowIfNull(arguments);

I'm a little worried about consistency. The other ctors for ProcessStartInfo are not throwing ArgumentNullException and Process.Start(string fileName, IEnumerable<string> arguments) is the only overload of that method that throws.

The other ctors for ProcessStartInfo are not throwing

This is true, but they are old and we can't easily change their behavior. For the new APIs, we either allow for nulls by making an argument nullable, or we don't and throw. Since the API was not approved as nullable (no ?), we should throw. By doing that we send a clear message in the API contract: nulls are not allowed, if you have a null arguments, just don't use this API.

adamsitnik · 2023-05-17T07:59:21Z

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

+            ProcessStartInfo psi = new ProcessStartInfo("filename", new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" });
+
+            Assert.Equal(6, psi.ArgumentList.Count);
+            Assert.Equal("arg1", psi.ArgumentList[0]);
+            Assert.Equal("arg2", psi.ArgumentList[1]);
+            Assert.Equal(" arg3", psi.ArgumentList[2]);
+            Assert.Equal("arg4 ", psi.ArgumentList[3]);
+            Assert.Equal("arg 5", psi.ArgumentList[4]);
+            Assert.Equal($"arg{Environment.NewLine}6", psi.ArgumentList[5]);


nit: it could be simplified

Suggested change

ProcessStartInfo psi = new ProcessStartInfo("filename", new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" });

Assert.Equal(6, psi.ArgumentList.Count);

Assert.Equal("arg1", psi.ArgumentList[0]);

Assert.Equal("arg2", psi.ArgumentList[1]);

Assert.Equal(" arg3", psi.ArgumentList[2]);

Assert.Equal("arg4 ", psi.ArgumentList[3]);

Assert.Equal("arg 5", psi.ArgumentList[4]);

Assert.Equal($"arg{Environment.NewLine}6", psi.ArgumentList[5]);

string[] args = new[] { "arg1", "arg2", " arg3", "arg4 ", "arg 5", $"arg{Environment.NewLine}6" };

ProcessStartInfo psi = new ProcessStartInfo("filename", args);

Assert.Equal(args, psi.ArgumentList);

adamsitnik · 2023-05-17T08:01:38Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs

+        /// <devdoc>
+        ///     Specifies the name of the application that is to be started, as well as a set
+        ///     of command line arguments to pass to the application.
+        /// </devdoc>


devdoc is relic of the past (it's not documented anywhere, you had no chance to know about it), please use standard summary tags

Suggested change

/// <devdoc>

/// Specifies the name of the application that is to be started, as well as a set

/// of command line arguments to pass to the application.

/// </devdoc>

/// <summary>

/// Specifies the name of the application that is to be started, as well as a set

/// of command line arguments to pass to the application.

/// </summary>

adamsitnik

Thank you for your contribution @jbhensley !

adamsitnik · 2023-05-18T13:30:06Z

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs

+        {
+            _fileName = fileName;
+
+            if(arguments != null)


The other ctors for ProcessStartInfo are not throwing

This is true, but they are old and we can't easily change their behavior. For the new APIs, we either allow for nulls by making an argument nullable, or we don't and throw. Since the API was not approved as nullable (no ?), we should throw. By doing that we send a clear message in the API contract: nulls are not allowed, if you have a null arguments, just don't use this API.

adamsitnik

We are almost there!

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs

src/coreclr/tools/SuperFileCheck/Program.cs

- Fix whitespace

adamsitnik

LGTM, thank you very much for your contribution @jbhensley !

danmoseley · 2023-05-19T14:16:36Z

@jbhensley if you are interested in continuing to contribute, we would welcome it. you may look at
https://github.com/dotnet/runtime/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22

if you are interested in adding API, look for 'api-approved' label too

Add ProcessStartInfo constructor that accepts IEnumerable<string> arg…

f52d103

…uments

dotnet-issue-labeler bot added area-System.Diagnostics.Process new-api-needs-documentation labels May 16, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label May 16, 2023

danmoseley reviewed May 16, 2023

View reviewed changes

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs Outdated Show resolved Hide resolved

Add more test scenarios

5c925c7

stephentoub reviewed May 16, 2023

View reviewed changes

Simplify collection initialization

dbf7815

adamsitnik reviewed May 17, 2023

View reviewed changes

adamsitnik self-assigned this May 17, 2023

adamsitnik added this to the 8.0.0 milestone May 17, 2023

jbhensley added 3 commits May 17, 2023 07:05

Fix API summary tags

e9c62f9

Simplify unit tests

7623449

Refactor various call sites to use new overload

bd45642

jbhensley requested review from thaystg and radical as code owners May 17, 2023 15:31

runfoapp bot mentioned this pull request May 17, 2023

Infra improvements for Helix #68176

Closed

build-analysis bot mentioned this pull request May 17, 2023

Test failure tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.sh #86233

Closed

adamsitnik requested changes May 18, 2023

View reviewed changes

ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 18, 2023

jbhensley added 2 commits May 18, 2023 07:01

Throw ArgumentNullException

1b5e494

Modify SuperFileCheck to use new ProcessStartInfo ctor

2e0a9a0

ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 18, 2023

build-analysis bot mentioned this pull request May 18, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

adamsitnik reviewed May 19, 2023

View reviewed changes

src/libraries/System.Diagnostics.Process/tests/ProcessStartInfoTests.cs Outdated Show resolved Hide resolved

src/coreclr/tools/SuperFileCheck/Program.cs Outdated Show resolved Hide resolved

- Revert calls to new overload.

356bd75

- Fix whitespace

adamsitnik approved these changes May 19, 2023

View reviewed changes

adamsitnik merged commit 4c51bc5 into dotnet:main May 19, 2023

jbhensley deleted the issue-66450 branch May 19, 2023 14:54

build-analysis bot mentioned this pull request May 20, 2023

Build ios-arm64 Release AllSubsets_Mono failures dotnet/arcade#13625

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

jbhensley commented May 16, 2023 •

edited

Loading

dotnet-issue-labeler bot commented May 16, 2023

ghost commented May 16, 2023

danmoseley commented May 16, 2023

stephentoub May 16, 2023

adamsitnik left a comment

adamsitnik May 17, 2023

jbhensley May 17, 2023

adamsitnik May 18, 2023

adamsitnik May 17, 2023

adamsitnik May 17, 2023

adamsitnik left a comment

adamsitnik May 18, 2023

adamsitnik left a comment

adamsitnik left a comment

danmoseley commented May 19, 2023

	public static Process Start(string fileName, IEnumerable<string> arguments)
	{
	ArgumentNullException.ThrowIfNull(fileName);
	ArgumentNullException.ThrowIfNull(arguments);

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

Add ProcessStartInfo constructor that accepts IEnumerable<string> arguments #86346

Conversation

jbhensley commented May 16, 2023 • edited Loading

dotnet-issue-labeler bot commented May 16, 2023

ghost commented May 16, 2023

danmoseley commented May 16, 2023

stephentoub May 16, 2023

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik May 17, 2023

Choose a reason for hiding this comment

jbhensley May 17, 2023

Choose a reason for hiding this comment

adamsitnik May 18, 2023

Choose a reason for hiding this comment

adamsitnik May 17, 2023

Choose a reason for hiding this comment

adamsitnik May 17, 2023

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik May 18, 2023

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

adamsitnik left a comment

Choose a reason for hiding this comment

danmoseley commented May 19, 2023

jbhensley commented May 16, 2023 •

edited

Loading