Expose NativeLibrary APIs #34567
Expose NativeLibrary APIs #34567
Conversation
| public static bool TryLoad(string libraryPath, out System.IntPtr handle) { throw null; } | ||
| public static bool TryLoad(string libraryName, System.Reflection.Assembly assembly, DllImportSearchPath? searchPath, out System.IntPtr handle) { throw null; } | ||
| } | ||
| } |
There was a problem hiding this comment.
Are there tests to go along with this?
There was a problem hiding this comment.
Tests are already in CoreCLR repo:
https://github.com/dotnet/coreclr/blob/master/tests/src/Interop/NativeLibrary/NativeLibraryTests.cs
There was a problem hiding this comment.
Does Mono reuse CoreCLR tests, like they do CoreFX tests? @marek-safar
There was a problem hiding this comment.
Does Mono reuse CoreCLR tests
Yes, Mono runs snapshot of CoreCLR tests. BTW: mono/mono#9640 tracks updating that snapshot
In general, a large part of interop test coverage is in CoreCLR. CoreFX does not have the specific infrastructure required to build and run interop tests.
There was a problem hiding this comment.
There are few places in CoreFX, where these APIs could be used:
https://github.com/dotnet/corefx/search?l=csharp&q=dlopen
There was a problem hiding this comment.
In particular, it'd be good to use System.Drawing as validation for this.
There was a problem hiding this comment.
I agree that it is a good idea to use the new API in CoreFX libraries. However,
- I don't think it is a good idea to make product changes to libraries in this checkin.
- There is currently no way to pass
RTDL_NOWtoNativeLibrary.Load()methods.
This should be investigated along with this issue.
If there are specific cases in CoreFx libraries that are not tested by NativeLibraryTests, I'm happy to extract them and add as test cases.
There was a problem hiding this comment.
I don't think it is a good idea to make product changes to libraries in this checkin
Right, we should have a follow up issue for it.
There was a problem hiding this comment.
| @@ -9,6 +9,7 @@ | |||
| </PropertyGroup> | |||
| <ItemGroup> | |||
| <Compile Include="System.Runtime.InteropServices.cs" /> | |||
| <Compile Condition="'$(TargetsNetCoreApp)' == 'true'" Include="System.Runtime.InteropServices.netcoreapp.cs" /> | |||
There was a problem hiding this comment.
In general, we have been keeping the API set the same between netcoreapp and uap. When the API was not implemented for ProjectN yet, we added it to the API compat baseline to be looked at later.
There was a problem hiding this comment.
I do not see a fundamental problem with implementing these APIs for UWP. They will be limited to what UWP allows you do, but that's ok. And we can always implement them by throwing PNSE if there is no better option.
Also, I think it is unlikely we would keep UWP as a separate targeting pack in the next major turn of the crank of that platform. I expect that we would just merge it with netcoreapp once that happens.
There was a problem hiding this comment.
I do not see a fundamental problem with implementing these APIs for UWP. They will be limited to what UWP allows you do, but that's ok
It's my understanding that all of these are wrappers over LoadLibrary which is unavailable in UWP due to WACK.
Though I just had a look at https://docs.microsoft.com/en-us/uwp/win32-and-com/win32-apis and noticed LoadPackagedLibrary is available (as well as GetProcAddress and FreeLibrary). I suppose I could imagine this being implemented on top of that for UWP.
it is unlikely we would keep UWP as a separate targeting pack in the next major turn of the crank
That's news to me. Don't get me wrong I'd love to see it happen but I'm unaware of such POR. I don't see it happening from a consumption side of things due to existing library ecosystem so as long as it remains a separate TFM we can provide a separate reference for it.
There was a problem hiding this comment.
There is no POR for next major turn of the crank for UWP today. I am just making a guess about what the POR is likely going to be once there is one.
There was a problem hiding this comment.
noticed LoadPackagedLibrary is available (as well as GetProcAddress and FreeLibrary
Right.
There was a problem hiding this comment.
I see. That actually makes this API specifically interesting for UWP where we haven't permitted dynamic load in the past. If we can provide a way to do it safely then I think this actually adds value there.
There was a problem hiding this comment.
Thanks for the explanation @ericstj. I'll change the contract to be visible everywhere, with implementation exceptions.
There was a problem hiding this comment.
Sure, sorry for the original poor advice. That was my misunderstanding.
Expose the System.Runtime.Interop.NativeLibrary APIs implemented in CoreCLR. API review: #32015 CoreCLR Change: dotnet/coreclr#21821 Tests for the API are here: https://github.com/dotnet/coreclr/blob/master/tests/src/Interop/NativeLibrary/NativeLibraryTests.cs
swaroop-sridhar
force-pushed
the
NativeLib
branch
from
2ea3fbf to
10f7ab1
Compare
Expose the System.Runtime.Interop.NativeLibrary APIs implemented in CoreCLR. API review: dotnet/corefx#32015 CoreCLR Change: dotnet/coreclr#21821 Tests for the API are here: https://github.com/dotnet/coreclr/blob/master/tests/src/Interop/NativeLibrary/NativeLibraryTests.cs Commit migrated from dotnet/corefx@5241ad4
Expose the System.Runtime.Interop.NativeLibrary APIs implemented in CoreCLR.
API review: #32015
CoreCLR Change: dotnet/coreclr#21821