C# Fix loading the native library on dotnet cli#7282
C# Fix loading the native library on dotnet cli#7282jskeet wants to merge 5 commits intogrpc:masterfrom
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
3 similar comments
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test. |
There was a problem hiding this comment.
From the stackoverflow post, this works for C __stdcall functions, which are common for 32 bits windows? Not sure if all of the 32bit windows libraries use __stdcall, but would there be C functions that the DLLImport could find that wouldn't match these names?
There was a problem hiding this comment.
Put it this way: running dumpbin /exports grpc_csharp_ext.dll showed everything being done this way.
Now it could be that if we change how the library is built, that would change what's exported...
(We could try also just finding the name without any prefix/suffix, of course.)
There was a problem hiding this comment.
Thanks, saw here https://msdn.microsoft.com/en-us/library/aa984739(v=vs.71).aspx that DLLImport was limited to __std_call anyways, so I think its the same then.
|
Not sure why some of the tests look to be unable to load the native library |
|
Will have a look over the weekend, or on Monday. Fingers crossed it's something simple... |
|
I've had a thought - I suspect it's due to where the Grpc.Core csproj files copies the native library - basically the code expects to find it as it's packaged in nuget. Fingers crossed that'll be easy to fix... |
|
Okay, that seems to have improved the tests - not sure why the basic and performance tests are failing though. The basic tests doesn't seem to be the C# ones anyway... |
|
Hi Jon, To make it easier to read I organized this in two parts, one for each test: The Basic testsThe reason the Basic tests failed is as follows, and mainly because of the
The reason for this is that the directory cannot be created for several reasons:
I do not have access to the Google Jenkins setup to help with troubleshooting it, and determine what might be the precise cause - or if it is something else - but below is a link to a detailed manual for Distributed Builds using Jenkins: The Performance smoke testThe reason the Peformance smoke test failed is as follows:
tools/jenkins/run_performance.sh And the actual line that runs it is this: tools/run_tests/run_performance_tests.py -l c++ node ruby csharp python --netperf --category smoketestIf the run_performance_tests.py script fails, it will cascade up to Jenkins to return the
The reason the above is failing, is because in line 603 of the src/python/grpcio/grpc/_server.py script, it tries to make self.generic_handlers = list(generic_handlers)A To actually create a list of objects, it might be best to append them as follows: >>> class MyClass:
... """A simple example class"""
... i = 12345
...
>>> listOfObjects = []
>>> listOfObjects.append(MyClass)
>>> listOfObjects.append(MyClass)
>>> listOfObjects
[<class __main__.MyClass at 0x7ffff7f0a6b0>, <class __main__.MyClass at 0x7ffff7f0a6b0>]
>>> type(listOfObjects)
<type 'list'>Hope it helps, |
There was a problem hiding this comment.
btw, this file is generated using a mako template
(e.g. you need to update the template and run tools/buildgen/generate_project.sh - otherwise the sanity test will fail to indicate that)
Feel free to look at my PR #7295 (I am updating templates/src/csharp/build_options.include there).
There was a problem hiding this comment.
Thanks - I suspect I haven't got everything I need on my Windows box to run that (as it's failing, in a way which is slightly annoying to debug due bash closing automatically on Windows...) Will look into it.
There was a problem hiding this comment.
Hi Jon,
I would just open a cmd.exe window, and launch a Bash shell via Cygwin as follows:
c:\cygwin\bin\bash.exe --login -i
Then proceed to where your Bash script is located, and run it manually as on a normal Linux/Unix box. The drives on Windows are located under /cygdrive like this:
$ ls /cygdrive
c
This way when running the script, you can see the debug information without having to worry that the window will close.
Hope it helps,
~p
There was a problem hiding this comment.
Well I don't have cygwin installed and don't wish to go that way - I've made some progress on Linux, although the script is now failing in boringssl. I have other more urgent demands at the moment, but will come back to this when I get a chance.
There was a problem hiding this comment.
No problem, we'll be here if you need us. Not sure if it's the same thing, but I've run into issues with BoringSSL previously, where I had to make a change to the Makefile to disable hidden visibility:
$ cp Makefile Makefile-orig
$ sed s/-fvisibility=hidden//g Makefile > Makefile-not-hidden
$ cp Makefile-not-hidden Makefile
I've posted the whole thing with more details under issue #7298
~p
There was a problem hiding this comment.
Thanks for the very quick response - will give that a try while I'm waiting for other build services to probably fail. Today is apparently "swear at infrastructure" day :(
There was a problem hiding this comment.
Worked it out now - I didn't have go installed. Looking better now. I suspect that CONTRIBUTING.md could do with being updated for the various dependencies required before running the toolchain. (I didn't have pyyaml or mako installed before today either...)
There was a problem hiding this comment.
Great! Glad to hear it's working out :)
|
Overall, I think the changes in this PR are reasonable. C# tests are passing, which is good (but currently they are only running with .NET and mono, not CoreCLR). We also have tests for distribution packages (the nugets) - but they have to be spawned manually as the build process is a bit more sophisticated: |
There was a problem hiding this comment.
You are not planning to use this property anywhere in this PR, right?
I think it would make sense to add it in a separate PR, as this property is only intended for expert use and for the things we are trying to fix here, no one should be relying on it.
There was a problem hiding this comment.
(in case you insist on adding this property here, please mark it as experimental API that can change any time - that's what I've done with other properties I wasn't 100% sure about)
There was a problem hiding this comment.
Will move it to a separate PR, and mark it as experimental as suggested.
|
Distribtest links: Experimental nugets with netstandard support are here: Distrib test results for the "official" nugets are here: |
The goal of this is to fix grpc#7230. This turned out to be a little trickier than I thought, but I'm happy with the result. The changes here are: - The layout in the nuget package; the files are now in /runtimes/{os-architecture}/native/{library} - 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. I have tested this on Windows with a console app: - Under dotnet cli, only on x64 (as running anything on x86 seems to be broken in dotnet cli at the moment) - Under msbuild/csproj, in both x64 and x86 - Under dnx, in both x64 and x86 I have *not* tried this on Linux or OSX, but those paths haven't changed much, so I'd expect them to work fine (or blow up immediately in CI if they fail; a failure here is unlikely to be subtle).
I suspect the differences in NativeDeps are the only ones required to get the tests working, but we'll probably have to change everything eventually. It's not clear why we have to specify the mappings in *so* many places though - we should look at tidying that up.
This means we'll get both x86 and x64 versions on deployment, which is a waste for environments where the right version would be picked, but fixes issues where the wrong architecture is picked due to dotnet cli tooling bugs. Also changed the project template, but I'll need to run the generators on a different machine as it looks like they don't work on Windows (at least without more effort).
|
LGTM |
6 participants
Two separate commits: