Refactor DllImports in Microsoft.Extensions.Hosting.Systemd #63421
Conversation
Refactor the DllImports in Microsoft.Extensions.Hosting.Systemd into Common/src/Interop following the interop guidelines.
ghost
added
the
community-contribution
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to this area: @dotnet/area-extensions-hosting Issue DetailsRefactor the DllImports in Microsoft.Extensions.Hosting.Systemd into This fixes part but not all of #28035
|
|
Thanks! I don't have merge permissions, so assuming everything passes, would someone be able to complete the PR on my behalf? Thanks! |
|
@MattKotsenas yes, we'll merge it when we get green CI. @eerhardt let me know if you have any comment about this change. |
| internal static partial class libc | ||
| { | ||
| [GeneratedDllImport(Libraries.Libc, EntryPoint = "getppid")] | ||
| internal static partial int GetParentPid(); |
There was a problem hiding this comment.
It would be nice to add this file next to
SystemNative_GetParentPid symbol from System.Native (see: https://grep.app/search?q=SystemNative_GetPid®exp=true&filter[repo][0]=dotnet/runtime). If I remember it correctly, we intentionally try not to import symbol from libc directly (except for tests). getppid was indeed an exceptional case.
There was a problem hiding this comment.
We try not to use SystemNative_* APIs from out-of-box assemblies like the Microsoft.Extensions.* assemblies as they're considered private APIs for the Microsoft.NETCore.App framework.
There was a problem hiding this comment.
I saw that libc was once removed from Interop.Libraries.cs 27bc21d#diff-c1db2d15cd6bf99da7a388eca0ae209ff70f2708d8c4ee6f10d11ad320bdb917, not sure if it makes sense to add it back for OOTB assemblies. (e.g. System.Drawing interop code is still maintained under its own hierarchy)
There was a problem hiding this comment.
We try not to use SystemNative_* APIs from out-of-box assemblies like the Microsoft.Extensions.* assemblies as they're considered private APIs for the Microsoft.NETCore.App framework.
It is even more of a stronger statement than "try not to". We explicitly can't. This is because the Microsoft.Extensions.* assemblies target netfx where the System.Native shim isn't present. (Note the netfx assembly would also be used on traditional Mono). And also new Extension assemblies can run on older .NET Core versions. In that case the System.Native shim will be present, but it won't have the new shim method.
So for assemblies that aren't in the Microsoft.NETCore.App shared framework, we can't use the System.Native shims.
MattKotsenas
deleted the
refactor/28035-Microsoft.Extensions.Hosting.Systemd
branch
The discussion in PR dotnet#63421 clarified that System.Native shims for UNIX APIs aren't appropriate for assemblies that don't ship as part of the Microsoft.NETCore.App framework. Updating the interop guidelines to capture that clarification.
* Clarify P/Invoke shims guidance for OOB assemblies The discussion in PR #63421 clarified that System.Native shims for UNIX APIs aren't appropriate for assemblies that don't ship as part of the Microsoft.NETCore.App framework. Updating the interop guidelines to capture that clarification. * Update docs/coding-guidelines/interop-guidelines.md Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
5 participants
Refactor the DllImports in Microsoft.Extensions.Hosting.Systemd into
Common/src/Interop in order to adhere to the interop guidelines.
This fixes part but not all of #28035