Runtime: Add ProcessStartInfo.StandardInputEncoding property

[madelson]

See detailed latest proposal in https://github.com/dotnet/corefx/issues/20497#issuecomment-312896302

Original Proposal

By default, the standard IO readers and writer for a process use Console.OutputEncoding and Console.InputEncoding respectively.

The ProcessStartInfo class provides StandardOutputEncoding and StandardErrorEncoding properties to allow callers to override the encoding for those. This simply changes the encoding passed to the StreamReader object which wraps the underlying process byte stream.

To more naturally support talking to a process that uses a different encoding than the current process's console encoding, we should add a new property to ProcessStartInfo:

public Encoding StandardInputEncoding { get; set; }

This would have the same semantics as the existing properties:

• Defaults to null, which means use the default encoding
• If specified and the input stream is not redirected, an exception is thrown on process start

[EDIT] Added latest proposal link by @karelz

[madelson]

@danmosemsft I see you added a label regarding the API. Is this something I can help with or something that needs to happen on your end?

[danmosemsft]

@madelson thanks for your suggestion. It next needs attention from the area owner @Priya91 . I know she's out this week. If you don't hear back early next week, I"d use @priya91 again to ping her.

[madelson]

@Priya91 thoughts?

[Priya91]

@madelson Sorry for the delay, the proposal makes sense to me. We need actual usage scenarios to consider adding an API to .NET, by that, we need code samples on how the API is intended to be used. Can you come up with an API proposal as described here

[madelson]

Thanks @Priya91 here's my attempt:

The .NET Process class defaults to using the Console.OutputEncoding and Console.InputEncoding properties to set the encoding for its StandardInput, StandardOutput, and StandardError properties.

However, sometimes this does not match the encoding that the underlying process expects, leading to issues particularly with non-ASCII characters.

To account for this, the ProcessStartInfo class contains two properties (StandardOutputEncoding and StandardErrorEncoding) which allow one to adjust the encoding used. Because there is no StandardInputEncoding property, however, this does not fully account for communicating with a process that uses a non-default encoding for all of its standard IO streams.

We should add the StandardInputEncoding property.

Rationale and Usage

While there are workarounds for this limitation (e. g. constructing a custom StreamReader on top of process.StandardInput.BaseStream), the lack of encoding customization for standard input is confusing and feels like a "hole" in the API.

Making things worse is the fact that the default encoding behavior actually varies somewhat between .NET Framework and .NET Core. According to the .NET Core docs:

In the .NET Framework on the desktop, the Process class by default uses Console encodings, which are typically code page encodings, for the input, output, and error streams. For example code, on systems whose culture is English (United States), code page 437 is the default encoding for the Console class. However, .NET Core may make only a limited subset of these encodings available. If this is the case, it uses System.Text.Encoding.UTF8 as the default encoding.

This makes it more difficult to write portable code using the Process class, because the default can vary.

Usage of the new property is simple and will follow the usage patterns of the two existing encoding properties. For example:

```c#
using (var process = new Process
{
StartInfo =
{
FileName = "...",
RedirectStandardInput = true,
StandardInputEncoding = Encoding.UTF8,
}
})
{
process.Start();
process.StandardInput.Write("輸入數據");
process.StandardInput.Close();
process.WaitForExit();
}

## Proposed API

The only proposed API change is a new property in the `ProcessStartInfo` class:

```c#
class ProcessStartInfo
{
    public Encoding StandardInputEncoding { get; set; }
}

Details

Semantically, this would have the same behavior as the other two stream encoding properties. For example:

• The default value (null) means use the default behavior for determining the encoding
• If an encoding is specified and the input stream is not redirected, an exception is thrown on process start

[EDIT] Added C# syntax highlight by @karelz

[Priya91]

Thanks @madelson ! Marking api ready for review.

[terrajobst]

Video

Looks good as proposed.

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/VppFZYG-9cA?t=6428 (5 min duration)

[madelson]

@karelz @terrajobst thanks this is cool! I'm glad you guys are on board with this enhancement request.

[danmosemsft]

@madelson do you want to submit a PR (with tests) for this?

[danmosemsft]

@madelson any interest?

[khellang]

I can take a stab at this if @madelson can't 😄

[madelson]

@danmosemsft sorry I never got a notification for your comment for some reason. Taking a look now.

[madelson]

@danmosemsft ok so I added the new property to ProcessStartInfo but when I tried to reference the new property from the tests project it breaks the build. Is there something special I have to do here?

[danmosemsft]

@madelson you need to make the edit under both src\ and ref\ folders next to each other. The tests compile against ref (it's like a header). If you are still stuck push your branch and share a link to it...

[madelson]

@danmosemsft I did try that but I'm still getting the same build error. Here's what I have so far: https://github.com/dotnet/corefx/compare/master...madelson:process-info-standard-input-encoding?expand=1

[danmosemsft]

What is the error? Your change looks fine (but please use alpha order)

[madelson]

@danmosemsft the test project build fails because it thinks the new property doesn’t exist:

Severity Code Description Project File Line Suppression State
Error CS0117 'ProcessStartInfo' does not contain a definition for 'StandardInputEncoding' System.Diagnostics.Process.Tests C:...\corefx\src\System.Diagnostics.Process\testsProcessTests.cs 1260 Active

---

For alpha order, do you mean of properties? Happy to make that change in the final version.

[danmosemsft]

I do not have a computer handy this week. @maryamariyan @anipik can one of your please pull this branch and help with the build error?

[maryamariyan]

@danmosemsft @madelson @Anipik I'll pull this branch today.

[safern]

The reason why you're seeing this error is because the tests are built against netstandard andSystem.Diagnostics.Process was shipped inbox in netstandard2.0. When building the tests they will build vs the netstandard.dll ref assembly that we shipped with this contract: https://github.com/dotnet/standard/blob/master/netstandard/ref/System.cs#L4033-L4100 which doesn't have this new API.

Your changes are adding this new API to the .netcoreapp2.1 and uap, so this API will be available only in those to frameworks. Since the tests are built vs netstandard, that is why you're seeing that failure.

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Process/ref/Configurations.props

I don't know how we would like to add this API as this contract was already shipped inbox for netstandard2.0. Should we add it only for netcoreapp2.1 and make the tests built vs netcoreapp also?

cc: @weshaggard

[danmosemsft]

Wes is out. Presumably as you say we just need a netcoreapp configuration to the tests. Aren't there others we already did this for? Eg we added more members to String!

[safern]

Wes is out. Presumably as you say we just need a netcoreapp configuration to the tests. Aren't there others we already did this for? Eg we added more members to String!

Yes we do it in System.Runtime for example

https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L177

So basically what you would need to do is create a ProcessTests.netcoreapp2.1.cs file and include it in the tests csproj when '$(TargetGroup)' == 'netcoreapp' or '$(TargetGroup)' == 'uap' and then in the /tests/configurations.props file add:

netcoreapp-Windows_NT;
netcoreapp-Unix;

And that will fix the problem, if we want to ship this API as part of netcoreapp2.1

[madelson]

Thanks @safern I will give that a try... yep this fixes the build issue. Thanks!