Skip to content

Workaround for Linux library loader bug #49

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

Merged
merged 1 commit into from
May 11, 2018
Merged

Conversation

tmat
Copy link
Member

@tmat tmat commented May 11, 2018

No description provided.

@tmat tmat merged commit edb5170 into master May 11, 2018
@tmat tmat deleted the LinuxLoaderWorkaround branch May 11, 2018 22:00
@bording
Copy link

bording commented May 22, 2018

@tmat Does this PR mean that the approach suggested for LibGit2Sharp to manually load the libraries doesn't work?

@tmat
Copy link
Member Author

tmat commented May 22, 2018

@bording It works on Windows but as it turned out it doesn't work on other platforms :(

@tmat
Copy link
Member Author

tmat commented May 22, 2018

I don't think LibGit2Sharp can do anything to fix this. The Core CLR needs to be fixed.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Meanwhile plugins (like msbuild tasks) loading native libraries can implement more sophisticated workaround that works well with multiple RIDs: #50.

@bording
Copy link

bording commented May 22, 2018

@bording It works on Windows but as it turned out it doesn't work on other platforms :(

That's really unfortunate. That massively negates the benefit of the work I've done in libgit2/libgit2sharp#1571 to handle all of the linux RIDs.

I don't think LibGit2Sharp can do anything to fix this. The Core CLR needs to be fixed.

Is there an issue filed to track getting this fixed?

Do you happen to know how mono works in this scenario? If it also works correctly, then my PR is still useful.

Meanwhile plugins (like msbuild tasks) loading native libraries can implement more sophisticated workaround that works well with multiple RIDs: #50.

Would there be some way to incorporate a custom AssemblyLoadContext into LibGit2Sharp, or would that always need to be in the assembly that's loading the LibGit2Sharp assembly?

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Do you happen to know how mono works in this scenario? If it also works correctly, then my PR is still useful.

Mono uses .config files for configuring the loading paths, which is yet another mechanism. I haven't tested it.

Would there be some way to incorporate a custom AssemblyLoadContext into LibGit2Sharp, or would that always need to be in the assembly that's loading the LibGit2Sharp assembly?

I thought about that and it would be very messy. Two alternatives:

  1. Do not use DllImport. Get function pointers to all native APIs manually. Since libgit2 has a lot of APIs this would be a lot of work.

  2. The assembly that's importing the native library (LibGit2Sharp) needs to be loaded into a custom load context. That means that any public APIs of that assembly would need to be called via Reflection. LibGit2Sharp also has a lot of APIs, so that's similar mess as [1].

Since SourceLink only needs a few functions that are called from the build tasks it was relatively easy to split it into 2 assemblies - one that has build tasks and loads the other via Reflection to a custom load context and the other then loads LibGit2Sharp into the same load context. The interface between these two assemblies is rather simple - half a dozen method calls, so easy to do via Reflection.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Is there an issue filed to track getting this fixed?

@jeffschwMSFT Is there an issue tracking improvements in loading native libraries in CoreCLR? When searching related CoreCLR github issues it seems like there are many problems in this area. Is there an umbrella issue that summarizes all of them and tracks proposed design?

@bording
Copy link

bording commented May 22, 2018

Mono uses .config files for configuring the loading paths, which is yet another mechanism. I haven't tested it.

Yeah, but it's really inflexible and you have to trick it into working with libraries in a folder (it only works if the folder begins with lib). There's also no way to choose from multiple libraries based on distro with it.

My goal with libgit2/libgit2sharp#1571 was to use the manual loading you introduced to avoid the need for the config file in mono as well, unifying how we handle loading the library for all platforms. That's why I was wondering if you knew how mono behaves.

Do not use DllImport. Get function pointers to all native APIs manually. Since libgit2 has a lot of APIs this would be a lot of work.

And that's exactly why I've never considering trying to go down the manual loading road before. You stating that DllImport would just use the manually-loaded library was a revelation! It turning out to not work in all cases is definitely putting a damper on things.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

You stating that DllImport would just use the manually-loaded library was a revelation! It turning out to not work in all cases is definitely putting a damper on things.

Yeah, sadly it only works on Windows (both .NET Core and .NET FX). It will probably work on Windows on Mono as well. Seems like a property of the Windows loader rather then CLR.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Not sure what to do about the plugin scenarios on Linux before Core CLR gets fixed. We can reintroduce setting the PATH when running on Linux/Mac to make it work but it's not safe when running in an environment like msbuild where multiple plugins can be loaded in parallel.

@bording
Copy link

bording commented May 22, 2018

We can reintroduce setting the PATH when running on Linux/Mac to make it work but it's not safe when running in an environment like msbuild where multiple plugins can be loaded in parallel.

Changing PATH doesn't work for Linux/Mac anyway. That was only something that worked on Windows.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Oh, ok :(. Then the only solution is for the consumers to implement the manual load context loading as I did in SourceLink.

@bording
Copy link

bording commented May 22, 2018

At this point I'm thinking it might be worth it to consider the function pointer approach just to be able to take control of things and actually get it working properly.

@tmat
Copy link
Member Author

tmat commented May 22, 2018

Maybe the pointer initialization code can be generated... Might be a quite a bit of work though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants