Skip to content

Update NativeLibrary.xml #11661

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

Closed
wants to merge 1 commit into from
Closed

Conversation

ShalokShalom
Copy link

This changes the parameter libraryPath into executableName

Reasoning:

The path is automatically handled by the abstraction of the API itself. The user is not supposed to provide a path:

That would simply be not sensical, considering macOS, Linux, etc, all have different paths to begin with.

The argument could never be multiple paths to all of them, and it's the executable name (that has to be on the PATH, obviously) that is our concern.

I hope this is fine this way, and I also corrected two small typos. Thanks

Summary

Describe your changes here.

Fixes #Issue_Number (if available)

This changes the parameter **libraryPath** into **executableName**

Reasoning: 

The path is automatically handled by the abstraction of the API itself. The user is **not** supposed to provide a path: 

That would simply be not sensical, considering macOS, Linux, etc, all have different paths to begin with. 

The argument could never be multiple paths to all of them, and it's the executable name (that has to be on the PATH, obviously) that is our concern. 

I hope this is fine this way, and I also corrected two small typos. 
Thanks
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 6, 2025
@jkoritzinsky
Copy link
Member

Changing the parameter name is a source-breaking change in C#. As a result, the documentation must match the source code (which uses the existing name). As a result, we cannot accept this sort of change.

@AaronRobinsonMSFT
Copy link
Member

Argument name changes are source breaking for C#. This would also require updating the source code and going through API review. I also don't agree with the argument being made to change this name. This API is intentially made for shared libraries, industry terminology, not simply "executables".

The path is automatically handled by the abstraction of the API itself. The user is not supposed to provide a path:

That is an option, but not required. In fact it is very common for relative paths to be used.

I hope this is fine this way, and I also corrected two small typos. Thanks

I would factor out the typos into a different PR.

@ShalokShalom
Copy link
Author

How are relative paths then supposed to be applied, so that they are cross platform compatible?

@AaronRobinsonMSFT
Copy link
Member

How are relative paths then supposed to be applied, so that they are cross platform compatible?

A common pattern is to do something like below. The IsOSPlatform() check is treated as an intrinsic by the JIT so there should be no runtime overhead in a Release build.

string path;
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
    path = "rsc\foo.dll";
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
{
    path = "rsc/libfoo.dylib";
}
else
{
    path = "rsc/libfoo.so";
}
NativeLibrary.Load(path);

@ShalokShalom ShalokShalom deleted the patch-1 branch August 7, 2025 15:06
@ShalokShalom
Copy link
Author

Do you agree, that it might be helpful, to include that information on the documentation page of the API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates that the PR has been added by a community member needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants