Overhaul how the native extension is found, loaded and used #7401

jskeet · 2016-07-15T06:12:32Z

The goal of this is to fix #7230.

The changes here are:

The layout in the nuget package; the files are now in
/runtimes/{os}/native/{library}
The filename of each library, which now includes the architecture,
e.g grpc_csharp_ext.x64.dll
The targets file used to copy those files in msbuild-based projects;
note that we now don't build up a folder structure.
The way the functions are found

Before this change, on Linux and OSX we used to find library symbols
manually, and use DllImport on Windows. With this change, the name
of the library file changes based on architecture, so DllImport
doesn't work. Instead, we have to use GetProcAddress to fetch the
function. This is further convoluted by the convention on
Windows-x86 to prefix the function name with _ and suffix it based
on the stack size of the arguments. We can't easily tell the
argument size here, so we just try 0, 4, 8...128. (128 bytes should
be enough for anyone.) This is inefficient, but it's a one-time hit
with a known number of functions, and doesn't seem to have any
significant impact.

The benefit of this in code is we don't need the DllImports any
more, and we don't need to conditionally use FindSymbol - we just
use it for everything, so things are rather more uniform and tidy.

The further benefit of this is that the library name is no longer
tied to a particular filename format - so if someone wanted to have
a directory with the libraries for every version in, with the
version in the filename, we'd handle that just fine. (At least once

Testing:

Windows:
- Console app under msbuild, dotnet cli and DNX
- ASP.NET Classic under msbuild
- ASP.NET Core (still running under net451)
Ubuntu 16.04
- Console app under dotnet cli, run with dotnet run and mono
OSX:
- Console app under dotnet cli, run with dotnet run and mono

Under dotnet cli, a dependency on Microsoft.NETCore.Platforms is
required in order to force the libraries to be copied.

This change does not further enable .NET Core. It attempts to
keep the existing experimental .NET Core project files in line
with the msbuild files, but I expect further work to be required
for .NET Core, which has a different build/publication model.

The goal of this is to fix grpc#7230. The changes here are: - The layout in the nuget package; the files are now in `/runtimes/{os}/native/{library}` - The filename of each library, which now includes the architecture, e.g `grpc_csharp_ext.x64.dll` - The targets file used to copy those files in msbuild-based projects; note that we now don't build up a folder structure. - The way the functions are found Before this change, on Linux and OSX we used to find library symbols manually, and use DllImport on Windows. With this change, the name of the library file changes based on architecture, so `DllImport` doesn't work. Instead, we have to use `GetProcAddress` to fetch the function. This is further convoluted by the convention on Windows-x86 to prefix the function name with `_` and suffix it based on the stack size of the arguments. We can't easily tell the argument size here, so we just try 0, 4, 8...128. (128 bytes should be enough for anyone.) This is inefficient, but it's a one-time hit with a known number of functions, and doesn't seem to have any significant impact. The benefit of this in code is we don't need the DllImports any more, and we don't need to conditionally use `FindSymbol` - we just use it for everything, so things are rather more uniform and tidy. The further benefit of this is that the library name is no longer tied to a particular filename format - so if someone wanted to have a directory with the libraries for every version in, with the version in the filename, we'd handle that just fine. (At least once Testing: - Windows: - Console app under msbuild, dotnet cli and DNX - ASP.NET Classic under msbuild - ASP.NET Core (still running under net451) - Ubuntu 16.04 - Console app under dotnet cli, run with dotnet run and mono - OSX: - Console app under dotnet cli, run with dotnet run and mono Under dotnet cli, a dependency on `Microsoft.NETCore.Platforms` is required in order to force the libraries to be copied. This change does *not* further enable .NET Core. It attempts to keep the existing experimental .NET Core project files in line with the msbuild files, but I expect further work to be required for .NET Core, which has a different build/publication model.

grpc-kokoro · 2016-07-15T06:15:07Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

grpc-kokoro · 2016-07-15T06:16:07Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

grpc-kokoro · 2016-07-15T06:17:19Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

grpc-kokoro · 2016-07-15T06:20:08Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

ivannaranjo · 2016-07-15T10:28:46Z

Can we change the way the Windows DLL is built to include an exports file? That will eliminate the issue you are having with the automatic names, you can export the functions with any name you want and isolating the calling code from this madness, see .def files here.

ivannaranjo · 2016-07-15T10:33:37Z

src/csharp/Grpc.Core/Internal/NativeExtension.cs

@@ -176,17 +178,18 @@ private static string GetArchitectureString()
        // platform specific file name of the extension library
        private static string GetNativeLibraryFilename()
        {
+            string architecture = GetArchitectureString();


You do the platform check twice, inside of GetArchitectureString() and here. Why not just move the architecture check here and return the hardcoded name of the library? The String.Format doesn't really gain much in this scenario.

No, GetArchitectureString only checks the 32/64-bit aspect; the only platform checks in here are for OS. Did you mean in GetPlatformString? I'm sure there are other ways of doing this, but I was aiming for minimal changes in this PR... this approach works, and the overhead is tiny and one-off.

ivannaranjo · 2016-07-15T10:37:16Z

Good heroic effort :). Sorry for the head aches.

jskeet · 2016-07-15T14:52:47Z

We may be able to change how the DLL is built in the future, but I don't want to start messing with that right now. The workaround for guessing the name should be fine - if something changes that stops it working, it will break all the tests. It's an implementation detail that can definitely be changed later.

jskeet · 2016-07-15T15:11:21Z

The "basic tests" failures don't look related to this change, as far as I can tell.

apolcyn · 2016-07-15T15:57:39Z

Seems to me too that the windows loading code could be simplified, but since its not missing out on anything that the DLLImport was getting (both covering stdcall names), this could be done separately. Made an issue for that here #7409

apolcyn · 2016-07-15T15:57:54Z

LGTM

jskeet · 2016-07-15T16:01:15Z

@apolcyn: Could you merge, please? I don't have commit permissions on this project.

apolcyn · 2016-07-15T16:27:57Z

Sorry as I haven't been merging things yet, I'm waiting on someone else to merge now.

jtattermusch · 2016-07-18T01:59:05Z

@ivannaranjo

I believe the native DLLs on windows are built in the right way - there's two ways to export the symbols - using the .def file or using __declspec(dllexport) (see here:

grpc/src/csharp/ext/grpc_csharp_ext.c

Line 49 in bbf2fcd

#define GPR_EXPORT __declspec(dllexport)

). AFAIK, both have the same effect and without exporting the symbols, there [DllImport] attributes we were using until this PR was merged wouldn't work.

ivannaranjo · 2016-07-20T10:14:22Z

You can specify the name of the exported symbol in the def file with the =
syntax, which is the whole point. If you are going to import the symbols by
name you need to specify the name, don't count on the @ notation for the
names of the exported symbols.

On Mon, Jul 18, 2016 at 2:59 AM, Jan Tattermusch notifications@github.com
wrote:

@ivannaranjo https://github.com/ivannaranjo

I believe the native DLLs on windows are built in the right way - there's
two ways to export the symbols - using the .def file or using
__declspec(dllexport) (see here:

grpc/src/csharp/ext/grpc_csharp_ext.c

Line 49 in bbf2fcd

#define GPR_EXPORT __declspec(dllexport)

).
AFAIK, both have the same effect and without exporting the symbols, there
[DllImport] attributes we were using until this PR was merged wouldn't work.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7401 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AJWmrAIQ0nHyzcXyATF-degrsf8gO3jZks5qWt37gaJpZM4JNG4y
.

googlebot added the cla: yes label Jul 15, 2016

ivannaranjo reviewed Jul 15, 2016
View reviewed changes

jskeet mentioned this pull request Jul 15, 2016

C# Fix loading the native library on dotnet cli #7282

Closed

apolcyn added the disposition/ready to merge label Jul 15, 2016

kpayson64 merged commit 24ae8ea into grpc:v1.0.x Jul 15, 2016

jtattermusch added the lang/C# label Jul 18, 2016

jtattermusch added the toolchain/.NET Core label Sep 7, 2016

apolcyn mentioned this pull request Sep 7, 2016

Test that both 'dotnet publish' and 'dotnet run' work well for users targeting netcoreapp. #8012

Closed

matt-kwong mentioned this pull request Dec 8, 2016

Make code coverage work again #8824

Closed

jtattermusch added lang/C# and removed lang/C# labels May 15, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul how the native extension is found, loaded and used #7401

Overhaul how the native extension is found, loaded and used #7401

jskeet commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

ivannaranjo commented Jul 15, 2016

ivannaranjo Jul 15, 2016

jskeet Jul 15, 2016

ivannaranjo commented Jul 15, 2016

jskeet commented Jul 15, 2016

jskeet commented Jul 15, 2016

apolcyn commented Jul 15, 2016

apolcyn commented Jul 15, 2016

jskeet commented Jul 15, 2016

apolcyn commented Jul 15, 2016

jtattermusch commented Jul 18, 2016

ivannaranjo commented Jul 20, 2016

Overhaul how the native extension is found, loaded and used #7401

Overhaul how the native extension is found, loaded and used #7401

Conversation

jskeet commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

grpc-kokoro commented Jul 15, 2016

ivannaranjo commented Jul 15, 2016

ivannaranjo Jul 15, 2016

Choose a reason for hiding this comment

jskeet Jul 15, 2016

Choose a reason for hiding this comment

ivannaranjo commented Jul 15, 2016

jskeet commented Jul 15, 2016

jskeet commented Jul 15, 2016

apolcyn commented Jul 15, 2016

apolcyn commented Jul 15, 2016

jskeet commented Jul 15, 2016

apolcyn commented Jul 15, 2016

jtattermusch commented Jul 18, 2016

ivannaranjo commented Jul 20, 2016