Remove redundant P/Invoke-s in S.D.Process on Apple platforms #54273
Conversation
…h doesn't exist on Apple platforms
MaximLipnin
requested review from
marek-safar,
directhex,
steveisok,
akoeplinger and
mdh1418
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-process Issue DetailsContributes to #47533. This PR excludes only P/Invoke to But, as Alexander mentioned, since all the native console methods in
|
| @@ -19,7 +19,10 @@ public partial class Process : IDisposable | |||
| private static volatile bool s_initialized; | |||
| private static readonly object s_initializedGate = new object(); | |||
| private static readonly ReaderWriterLockSlim s_processStartLock = new ReaderWriterLockSlim(); | |||
|
|
|||
| #if !TARGET_MACCATALYST && !TARGET_IOS && !TARGET_TVOS | |||
There was a problem hiding this comment.
nit: is there a simpler way of saying that given target is from apple but it's not macOS?
There was a problem hiding this comment.
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: <IsiOSLike Condition="'$(TargetsMacCatalyst)' == 'true' or '$(TargetsiOS)' == 'true' or '$(TargetstvOS)' == 'true'">true</IsiOSLike> ?
There was a problem hiding this comment.
Also, you can avoid the ifdefs in the .cs files by moving the ConfigureTerminalForChildProcesses support into a separate .cs file, defining partial methods for it here, and including the .cs file only for targets that need it.
There was a problem hiding this comment.
Another alternative to solve this problem is to define a dummy method in the shim to make it link succesfully. This strategy is used in number of places today, e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/Native/Unix/System.Native/pal_interfaceaddresses.c#L571
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
Outdated
Show resolved
Hide resolved
| @@ -1051,26 +1050,9 @@ private static void OnSigChild(int reapAll) | |||
| /// </summary> | |||
| internal static void ConfigureTerminalForChildProcesses(int increment) | |||
There was a problem hiding this comment.
I do not think you need the ConfigureTerminalForChildProcesses / ConfigureTerminalForChildProcessesInner split. ConfigureTerminalForChildProcesses can be the partial method.
There was a problem hiding this comment.
The ConfigureTerminalForChildProcesses method has a modifier so there can be error CS8795: Partial method 'Process.ConfigureTerminalForChildProcesses(int)' must have an implementation part because it has accessibility modifiers.
Removing the modifier leads to src/System/Diagnostics/ProcessWaitState.Unix.cs(575,33): error CS0122: 'Process.ConfigureTerminalForChildProcesses(int)' is inaccessible due to its protection level
...iagnostics.Process/src/System/Diagnostics/Process.Unix.ConfigureTerminalForChildProcesses.cs
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Contributes to #47533.
This PR excludes only P/Invoke to
SystemNative_ConfigureTerminalForChildProcesswhich is mentioned in #47533.But, as Alexander mentioned, since all the native console methods in
src/libraries/Native/Unix/System.Native/pal_console.care excluded from the build on Apple platforms, it'd make sense to update the respective part of managed code as well.