Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using Mono.Posix in test code instead of directly P/Invoking to libc #63811

Closed
eerhardt opened this issue Jan 14, 2022 · 17 comments
Closed
Assignees
Labels
area-Meta test-enhancement Improvements of test source code

Comments

@eerhardt
Copy link
Member

Our current recommendation to customers who need to call into a POSIX API directly is to use the Mono.Posix nuget package. But we don't follow that advice ourselves in our tests. Instead we directly P/Invoke into libc in quite a few places:

private static partial class Libc
{
[GeneratedDllImport("libc", SetLastError = true)]
public static unsafe partial uint geteuid();

internal static partial class Interop
{
[DllImport("libc")]
internal static extern int getsid(int pid);

public abstract partial class FileSystemTest
{
[DllImport("libc", SetLastError = true)]
protected static extern int geteuid();
[DllImport("libc", SetLastError = true)]
protected static extern int mkfifo(string path, int mode);

[DllImport("libc", SetLastError = true)]
private static extern int kill(int pid, int sig);

We should consider replacing these direct P/Invokes with Mono.Posix.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 14, 2022
@ghost
Copy link

ghost commented Jan 14, 2022

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

Issue Details

Our current recommendation to customers who need to call into a POSIX API directly is to use the Mono.Posix nuget package. But we don't follow that advice ourselves in our tests. Instead we directly P/Invoke into libc in quite a few places:

private static partial class Libc
{
[GeneratedDllImport("libc", SetLastError = true)]
public static unsafe partial uint geteuid();

internal static partial class Interop
{
[DllImport("libc")]
internal static extern int getsid(int pid);

public abstract partial class FileSystemTest
{
[DllImport("libc", SetLastError = true)]
protected static extern int geteuid();
[DllImport("libc", SetLastError = true)]
protected static extern int mkfifo(string path, int mode);

[DllImport("libc", SetLastError = true)]
private static extern int kill(int pid, int sig);

We should consider replacing these direct P/Invokes with Mono.Posix.

Author: eerhardt
Assignees: -
Labels:

area-Meta

Milestone: -

@ericstj ericstj added test-enhancement Improvements of test source code and removed untriaged New issue has not been triaged by the area owner labels Jan 14, 2022
@ericstj
Copy link
Member

ericstj commented Jan 14, 2022

Looks like there are actually a lot more instances. https://github.com/dotnet/runtime/search?l=C%23&p=2&q=libc

@eerhardt what's the benefit we get out of moving to mono.posix? Is it that it handles platform differences that might exist between different Unix's? Or does it just save us from a bit of interop?

@eerhardt
Copy link
Member Author

what's the benefit we get out of moving to mono.posix?

The main benefit is dogfooding our own recommendations. If we point our customers to a solution, we shouldn't use a different solution for ourselves. It will also test Mono.Posix works in all the environments we test on.

Is it that it handles platform differences that might exist between different Unix's?

This is a benefit for sure. It is the same reason we created the System.Native shims.


One drawback to this approach is that if we are trying to bring up a new platform and Mono.Posix doesn't yet support it, it will block running tests. I'm not sure how often this happens, but it may affect our decision here.

@tmds
Copy link
Member

tmds commented Jan 19, 2022

if we are trying to bring up a new platform and Mono.Posix doesn't yet support it

Also existing platforms, like s390x, are not supported by Mono.Posix.

@danmoseley
Copy link
Member

Also existing platforms, like s390x, are not supported by Mono.Posix.

This itself is an example of the benefit of dogfooding - I wasn't aware of this.

@tmds
Copy link
Member

tmds commented Jan 19, 2022

I'd prioritize minimizing dependencies to build and test .NET over dogfooding Mono.Posix.

@tmds
Copy link
Member

tmds commented Jan 19, 2022

We ran the tests when adding s390x support. It would have been a hassle if we first needed to build Mono.Posix to be able to run the tests.

It's another dependency to deal with when porting .NET, and it is not bringing much.

@danmoseley
Copy link
Member

@steveisok this is the issue I mentioned -- are s390x folks potentially interested in making it possible to use Mono.Posix there?

@danmoseley
Copy link
Member

@tmds just curious, what's the issue -- big endian? Are there other platforms you know of that would need attention?

There's an argument perhaps for bringing Mono.Posix into this repo.

@ericstj
Copy link
Member

ericstj commented Jan 21, 2022

just curious, what's the issue -- big endian?

I'd like to hear from @tmds, but I suspect the issue is that Mono.Posix requires a native library and there is no binary reuse from an existing RID that Mono.Posix supported (since it's a new architecture).

There's an argument perhaps for bringing Mono.Posix into this repo.

If we brought Mono.Posix into dotnet/runtime we should use it. I wonder how it might co-exist with our existing System.Native shims and various PALs. Should we consolidate on the managed API it exposes? Or maybe on only the native modules? What sort of compat garuntees can we make for those (previously we said native shims have no compat promise).

@steveisok
Copy link
Member

/cc @akoeplinger @directhex

@jkotas
Copy link
Member

jkotas commented Jan 21, 2022

the issue is that Mono.Posix requires a native library and there is no binary reuse from an existing RID that Mono.Posix supported

Yes, and unlike some other ecosystem, .NET does not have a standard low friction mechanism to fill in the missing binary. Folding Mono.Posix into dotnet/runtime solves the problem for this one library. All other NuGet packages that bundle unmanaged binary are still going to have the problem.

I wonder how it might co-exist with our existing System.Native shims and various PALs. Should we consolidate on the managed API it exposes?

Majority of System.*.Native shims either do not expose Posix API or expose Posix API wrapped in an extra logic. There is not that many shims that expose pure Posix API. Having a bit of duplication for these shims is not a big deal.

@danmoseley
Copy link
Member

If I understand correctly then folding it would have value as we take care of the native library issue. Every managed library can then assume POSIX APIs are available to them.

The problem of packaging native libraries with managed libraries is a separate one to solve separately.

@jkotas
Copy link
Member

jkotas commented Jan 21, 2022

Every managed library can then assume POSIX APIs are available to them.

I do not expect we would include Mono.Posix into netcoreapp. It is not that useful API to be in the core platform by default. It would still be separate package and other libraries would still have to make a conscientious decision whether it is worth it for them to reference it to get what they need.

@tmds
Copy link
Member

tmds commented Jan 21, 2022

I suspect the issue is that Mono.Posix requires a native library and there is no binary reuse from an existing RID that Mono.Posix supported (since it's a new architecture).

Yes, you need to build this native library, and then package it up, potentially using a rid not known by the tooling (like the NuGet client libraries).
Then, somehow, you need to provide your custom built package to the dotnet/runtime build.

For someone else to run tests on the architecture, they need to get your custom built package until the official package adds support (if it ever does).

A big part of what source-build solves is to deal with dependencies. Minimizing dependencies, especially native ones, reduces the effort and complexity. I think it is more important than what we get by adopting Mono.Posix in tests.

@ericstj
Copy link
Member

ericstj commented Jan 25, 2022

Some good points were raised here about why to not take this dependency. @eerhardt do you still feel like this is something we should do?

@eerhardt
Copy link
Member Author

I'm fine with closing this. For the s390x support I opened mono/mono.posix#14 just so it is captured in Mono.Posix.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta test-enhancement Improvements of test source code
Projects
None yet
Development

No branches or pull requests

6 participants