-
Notifications
You must be signed in to change notification settings - Fork 897
Move native binaries into separate NuGet package #984
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
Conversation
Hmm, looks like the Travis CI builds didn't find the myget feed to get the NativeBinaries package, but AppVeyor didn't have a problem. Looking into it... |
Well, now it's working on Windows and Mac, so that's progress at least! |
Indeed. How about tweaking Regarding the naming of the binaries, I'd very like to keep the short sha suffix. Besides the troubleshooting help, it also allows two different version of LibGit2Sharp to be ran side by side in the same process, each of them leveraging a different version of libgit2 (eg. Visual Studio extensions). |
👍
👍 |
The updated package should have fixed the linux build, but nuget.org seems to be returning an error for the latest version of my package. All the builds failed because the couldn't restore the package. I'm looking into what's up with that now. The reason the linux Travis CI build was failing was that the linux library I included in the 0.0.2 version of my package was expecting the http-parser lib to be on the system. I rebuilt it using the internal fallback dependency, so once this nuget.org problem gets resolved, I expect all the builds to pass. |
If you look at the build log for the linux travis CI build for 093593b, you'll see that it's failing on the shadow copy test. As best I can tell, mono seems to have a problem with DLLImport and shadow copy assemblies, but only on linux. OS X appears to work fine. As a workaround, I'm about to commit a change to |
I realized why this is needed now: the path windows is looking for can be changed at runtime, but DLLImport, being an attribute, needs a single compile-time value. So, yeah don't see a simple way to remove this. 😞 |
That makes sense. I hadn't thought about needing multiple versions in the same process! Keeping that around means that The NativeBinaries package could include Updating I had been thinking that the So, this is what the scenario around updating LibGit2Sharp to a new version of libgit2 would look like:
|
And of course the linux build fails again... but this time it looks like nuget.org timed out so it couldn't restore the packages successfully. 😢 The 0.0.5 version of the package is the first version I've built from a nuspec instead of just manually putting it together. 😄 The repo I have where this is coming from is here It's close to being done, so take a look! I still need to add scripts to build and pack from linux/osx, but otherwise I think it's about done. My thought was that we could take what I have there and either copy or fork it over to an "official" repo once this is all done. |
You're doing an amazing work! ❤️
I've tried to restart the build, but it looks like I can't connect to Travis now. I'll try again later.
Couldn't we rely on some compile time trickery in the LibGit2Sharp side and dynamically regenerate the file (à la #438)? |
Regarding the NativeBinaries package content, would there be a way to rely on the CI servers to actually build them? We're already doing sort of this for LibGit2Sharp with AppVeyor (on merge, the NuGet package is published as a build artifact). Provided we'd do the same for the Windows builds of libgit2 (and find a way to retrieve the Travis Linux/OS produced binaries), the only manual operation would be to download the Windows only package, the Linux/Mac Os X binaries, update the package with those and publish it. |
@bording Just restarted the Mac Os X build Regarding the native package version, how about applying the same LibGit2Sharp scheme and end up with something like |
Thanks for linking to that! I had wondered what the UniqueIdentifier.targets was about, but hadn't had time to investigate yet. Any idea if that CLR bug ever got fixed in a public release? I think using a custom task like that for After adding another task to the
That sounds like a good idea to me. Getting a windows Nuget package out of Appveyor and the other binaries out of a Travis build and manually adding them in would mean that we wouldn't need to commit the binaries to the repo at all. I like it! 😄
Sure, should be possible. At the moment the package build command just takes any version string from the command line, which I've just been manually bumping up. I'll look into this as well. |
FYI, I have the NativeDllName custom build task working! I just need to get it cleaned up a bit before it's ready to be pushed up. |
Wow, Travis sure seems flaky, the nuget package restore failed again. 😢 |
Duh, I cannot connect to Travis. @svenfuchs Any idea? |
Build has been restarted |
<Target Name="GenerateNativeDllNameCs" | ||
Inputs="@(EmbeddedResource)" | ||
Outputs="$(NativeDllNamePath)"> | ||
<GenerateNativeDllNameTask InputHashFile="@(EmbeddedResource)" Condition=" '%(Filename)' == 'libgit2_hash' " OutputFile="$(NativeDllNamePath)" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be libgit2_hash.txt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm. scratch this last comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I had commented on this, but seems like it I must not have saved it. oops! Like I'm guessing you noticed, the %(Filename) metadata doesn't include the extension. I could go ahead and add %(Ext) in there too, just to make the comparison more complete.
I've made a bit more progress on my NativeBinaries repo. I've split the updatelibgit2 script out into a script that updates the submodule and related files, and now have a separate script that builds the windows binaries. I've also added a script for building the linux and osx binaries, but I still need to get some more logic in place to copy them to the correct subfolders. I suppose that might not matter though, if we can just push them out as build artifacts from the CI server. I still need to write travis.yml and appveyor.yml files for that repo, but otherwise I think this is close to being ready! |
@@ -37,8 +37,8 @@ _ReSharper*/ | |||
*.DotSettings | |||
#Ignore custom generated files | |||
LibGit2Sharp/Core/UniqueIdentifier.cs | |||
LibGit2Sharp/Core/NativeDllName.cs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we start ignoring it, shouldn't we also drop it from the index?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, thought 76eaeea included that, or is there something more that you're wanting me to do here?
I've got some more changes in NativeBinaries:
Things that still need to be done:
I think that covers everything I've got on my TODO list, but I do have a few questions.
|
I have finalized the unix build script, and I've been playing with CI setups, with free accounts pointing at my repo. I've added a travis.yml file, and it seems to be working fine, though apparently I can only get linux builds. 😦 To get artifacts pushed out of Travis, it looks like we have to set up some sort of 3rd party service, the default being an S3 account. I haven't added an AppVeyor.yml yet, because I've still been directly using the settings from the site. The problem I'm running into there is that the hardware I get with a free account must be very slow. The builds are timing out after running for 40 minutes! It's possible that something else is wrong, but I'm not getting much to go on yet. |
Just pushed up some more commits to my NativeBinaries repo. That cleans up the output of the Aside from finishing up the CI stuff, and the issues I mentioned above with that, this is just about done. @nulltoken Have you had chance to look over the stuff I've done recently? What about the questions I posted above? |
And, I just got the AppVeyor issue I was having sorted out, so I'll have an appveyor.yml ready to go soon also! Should be able to get it finalized and pushed up tomorrow. |
@bording Sorry for the delay in answering. I've just created the https://github.com/libgit2/libgit2sharp.nativebinaries repository. Could you please submit your work over there? I've requested multi os support from Travis for this repository. Regarding S3, @carlosmn seems to think that we may have other alternatives. |
# Exporting a dummy value with ':' works around a linux mono bug where it | ||
# can't find libgit when the libgit2sharp assembly has been shadow copied. | ||
# This appears to only be a problem on linux and not osx. | ||
export LD_LIBRARY_PATH=: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This neither a dummy value nor a bug in mono. It is not mono who is interpreting this value, but the system's loader, which is responsible for loading code into the application's memory space, and which may behave differently across OSs. On OS X, the current directory is searched by default, on Linux it is not. By setting an (or really, two) empty path as one of the paths to search, you're asking the loader to also search in the current path. Take a look at your system's dlopen(3)
manpage for details about how it behaves on your platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to be contrary, but all the research I've done so far seems to indicate something more is going on!
If I comment out that LD_LIBRARY_PATH line, the native binaries are still loaded by mono, up until the shadow copy test runs.
This mono documentation points out that mono looks in the referencing assembly's directory before it then relies on the system's loader to find it:
If a library location has not been explicitly specified in a DllMap entry in an application or assembly .config file, Mono will search for a library in a few places:
- The directory where the referencing image was loaded from.
- In any place the system’s dynamic loader is configured to look for shared libraries. For example on Linux this is specified in the $LD_LIBRARY_PATH environment variable and the /etc/ld.so.conf file. On windows the $PATH environment variable is used, instead.
I've seen that exact behavior also, on both osx and linux. The native libraries are found and used without needing any sort of LD_LIBRARY_PATH/DYLD_LIBRARY_PATH overrides set. The only time this is failing is on linux when the managed assemblies have been shadow copied.
I even took at look at mono's loader code, and found the section where they check the for the directory where the managed assembly lives before they go relying on dlopen
to find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been playing around with this a bit more, and I think I see what's going on. I wasn't aware that setting LD_LIBRARY_PATH to : would be treated as the current working directory, so adding that bit of info, combined with what I mentioned above, just might fully explain all the behavior I've been seeing!
When no shadow copying is going on, mono's built-in lookup option is finding the native library next to libgit2sharp.dll and loading it, without needing LD_LIBRARY_PATH.
However, when shadow copying is involved, the built-in lookup fails because it's looking in the shadow copy directory, and the native library isn't there.
It then has to fall back to the system-level loading, which is where the difference between OS X and linux that I was seeing comes into play. OS X does include the current working directory in its search path, but linux does not.That is why OS X works without having anything extra set.
Linux, however, needs some extra help, which is where my inadvertent setting of the current working directory value comes in, making it work there too.
Now, for all of this to be true, that means xbuild has to be changing the current working directory as it is running, and then resetting once it's finished, because the current working directory would have to be set to the directory where the native libraries are sitting for this scenario to be feasible.
I wasn't aware that it would be doing that, but I suppose it could be. If it is, then all of the behavior I've been seeing is explained.
My command about S3 was more about that simply being the default, but yeah, it may make sense to do something else. Who is supposed create the package with the native code? My understanding was that a libgit2sharp build of vNext/master would create the native packages and upload them, so I don't quite see why you want to store the results in some other place beforehand. |
@Therzok The build scripts haven't been removed, they've just been moved over to the native binaries repo instead. The entire libgit2 submodule has been removed from this repo and moved there, so keeping the build scripts here doesn't really make sense. I'd be happy to help get the SSH PR up and working over there, but the whole point of this PR is get the native stuff out of the LibGit2Sharp repo! 😄 |
Oh, great. That's fine by me. :D |
Just rebased this to vNext in preparation for updating it to use the almost ready real native binaries package! |
Ok, this is now using the real package! Unless there is any other feedback, I think this is ready to be squashed and merged! |
Yeah! Thanks to you. You've done an impressive job!
Looks perfect on my side. Please squash those commits. However, if that's ok with you, before merging this, I'd just like to wait until #1049 passes in order to ensure that everything is 👍. |
Squashed and rebased to vNext!
👍 |
@bording Small nitpick
Is there any strange non printable char in the file so that it's detected as binary? |
@nulltoken I noticed that too, and it turns out the Lib folder has a .gitattributes file that make it treat everything in there as binary. |
@bording Yep. That's it! I forgot about that (and I may be the guy that put it here in the first place 😉). BTW, not willing to derail this PR but while reading http://www.cazzulino.com/xplat-msbuild.html I was starting to wonder if we shouldn't try and got the same way.
Thoughts? /cc @Therzok |
@nulltoken The idea of being able to use the Inline Code Tasks would be a good option for getting rid of the CustomBuildTasks binary in the lib folder. I wonder if the cross platform MSBuild stuff is far enough along to actually be relied on since they haven't officially released anything yet. Another option would be to move CustomBuildTasks out into another nuget package, and LibGit2Sharp could use it as a development-only dependency. Thinking a bit further ahead, as I mentioned in #1019, I've started a branch playing around with moving to the new dnx-style project format, and trying to get a coreCLR build working. There are some pretty cool aspects to it, though several things will have to be revised. MSBuild tasks, for example, aren't going to work. |
I'd say that MSBuild has probably a good history of being "production ready" on Windows. 😉 With @kzu working for Xamarin, there might be a good chance it's been tested as well on *nix paltforms. The only thing that worries me a bit is that the NuGet package is actually maintained by @kzu and not someone from the MSBuild project. /cc @AndyGerlicher |
FYI, a PR was merged to the MSBuild repo yesterday that adds NuGet packages: dotnet/msbuild#95. They are now used to build dotnet/corefx on Unix (but I think they are only on corefx's myget feed and not on nuget.org yet). |
Fixes #739 |
Move native binaries into separate NuGet package
@bording Another fine piece of software engineering 💟 |
Published as NuGet pre-release package |
As discussed in #974, here is what I have done so far.
I'm currently hosting a manually created version of the native binaries package on a myget feed, which I've temporarily added as a solution-level package source, which should let package restore find the package without needing anyone to manually add the package source.
There are still several things that need to done still. The primary thing is deciding what to do with how handle the things that were previously being done by the
UpdateLibgit2ToSha.ps1
script:Core\NativeDllName.cs
(used for theDllImport
attributes)libgit2_hash.txt
(used byVersion.cs
)These both are related to how naming is currently handled for the native binaries, mainly trying to use the commit hash as part of the name. Since the binary would be coming from outside the project, keeping this aspect in place might be more trouble than it's worth. It might make more sense to just standardize the name.
I'd also like to look into changing the way
NativeMethod
s is looking for the binary file. Right now, on windows it's digging down into the NativeBinaries subfolder and then the architecture folder. For linux and osx, it's not doing anything special, just letting mono find it in the standard places.Is there a reason for needing the subfolders on windows? Why not have
libgit2-x86.dll
andlibgit2-x64.dll
and let them live side by side withlibgit2sharp.dll
? It looks like there would be some mono-specific workaround code that could be removed fromNativeMethods
if we just let all the native binaries live in the same folder!