8000 Libgit2 dll naming by ben · Pull Request #430 · libgit2/libgit2sharp · GitHub
[go: up one dir, main page]
Skip to content

Libgit2 dll naming #430

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 21 commits into from
Closed

Libgit2 dll naming #430

wants to merge 21 commits into from

Conversation

ben
Copy link
Member
@ben ben commented May 13, 2013

This adds automation and marshaling junk so that two different versions of Libgit2Sharp won't have conflicting copies of git2.dll in memory at the same time. This should help with #241.

@ben
Copy link
Member Author
ben commented May 13, 2013

I originally started with a T4 template, but it appears that the template doesn't re-build its target even on a clean build. I moved the (rather simple) content of the generated file into the powershell script, so it's totally guaranteed to be updated when you build libgit2 with that script.

dahlbyk
dahlbyk reviewed May 13, 2013
View reviewed changes
LibGit2Sharp/LibGit2Sharp.csproj
@@ -258,6 +259,9 @@
<ItemGroup>
<EmbeddedResource Include="libgit2sharp_hash.txt" />
</ItemGroup>
<ItemGroup>
<Service Include="{508349B6-6B84-4DF5-91F0-309BEEBAD82D}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a letfover from the T4 tryout. You're right, it should be removed.

@nulltoken
Copy link
Member

@ben This looks awesome! Just a final nitpick: Travis is failing because the Mono build also seeks for a git2_b641c00 binary.

How complex would it be to tweak the build.libgit2sharp.sh script so that it also appends the short sha to the generated binaries?

/cc @cmn

@nulltoken nulltoken mentioned this pull request May 13, 2013
Closed
@nulltoken
Copy link
Member

How complex would it be to tweak the build.libgit2sharp.sh script so that it also appends the short sha to the generated binaries and debug symbols?

Hmmm. Thinking a bit about this, I don't think this would actually be easy to tackle (or even makes sense) because of the linking between the [.so|.dylib] files.

Maybe would it be easier, for now, to add a processor directive in NativeDllName.cs

#if NOT_WIN
   private const string libgit2 = "git2";
#else
   private const string libgit2 = "git2_b641c00";
#endif

and pass /property:DefineConstants="NOT_WIN" to the xbuild call from the bash script.

Thoughts?

@nulltoken
Copy link
Member

An even better proposal may be to rely on the Mono provided <dllmap/> redirection mechanism. This would require to create a LibGit2Sharp.dll.config file with the following content:

<configuration> 
  <dllmap os="!windows,osx" dll="git2_b641c00" target="git2.so"/> 
  <dllmap os="osx" dll="git2_b641c00" target="git2.dylib"/> 
  <dllmap os="windows" dll="git2_b641c00" target="git2_b641c00.dll"/> 
</configuration>

Provided this is the chosen solution, UpdateLibgit2ToSha.ps1 would have to update this file as part of its job.

References:

@nulltoken
Copy link
Member

Another counter proposal: Tweak the libgit2 CMakeList to make it accept an optional suffix. This would make CMake generate git2-{suffix}.[dll|so|dylib] binary, depending on the target platform.

/cc @carlosmn

@nulltoken nulltoken mentioned this pull request May 13, 2013
Merged
@martinwoodward martinwoodward mentioned this pull request May 13, 2013
Merged
@nulltoken
Copy link
Member

Ok. I've pushed a counter proposal in nulltoken/vNext which relies on the NOT_WIN preprocessor constants.
This should fix the Windows side of the problem.

Can anyone please give a test at this and c 8000 onfirm that this fixes the issue?

/cc @xpaulbettsx @martinwoodward

@anaisbetts
Copy link
Contributor

✨

@nulltoken
Copy link
Member

So proposed plan is:

  • If nulltoken/libgit2sharp@7dda24368bbcf9a016179dedb5dab14fc3c77917 passes @xpaulbettsx test -> Tag it as v0.11.1 and release a new version of the NuGet package. Otherwise... Try again.
  • Once CMake accepts an optional suffix (@carlosmn 😉), remove the NOT_WIN hack so that Mono and .Net assemblies are the same again.

@anaisbetts
Copy link
Contributor

Working on this now, will report back soonish

@anaisbetts
Copy link
Contributor
anaisbetts co 8000 mmented May 13, 2013

No dice, here's what I did:

  1. Fetched to latest vNext on nulltoken/libgit2sharp
  2. Artificially bumped the version to 0.11.0.5
  3. Built a NuGet package
  4. Added the local dir as a source, ran Upgrade-Package in SaveAllTheTime
  5. Reinstalled the TFS Git provider
  6. Rigged IsTFSGitPluginInstalled to return false

Here's what shows up in the logs:

2013-05-13 15:49:17.9926 - WARN: Couldn't read status for repo: C:\Users\Paul\GitHub\SaveAllTheTime
EXCEPTION: System.Runtime.InteropServices.MarshalDirectiveException: FilePathMarshaler must be used on a FilePath.
   at LibGit2Sharp.Core.FilePathMarshaler.MarshalManagedToNative(Object managedObj)
   at System.StubHelpers.MngdRefCustomMarshaler.ConvertContentsToNative(IntPtr pMarshalState, Object& pManagedHome, IntPtr pNativeHome)
   at LibGit2Sharp.Core.NativeMethods.git_repository_open(RepositorySafeHandle& repository, FilePath path)
   at LibGit2Sharp.Core.Proxy.git_repository_open(String path)
   at LibGit2Sharp.Repository..ctor(String path, RepositoryOptions options)
   at SaveAllTheTime.Models.GitRepoOps.<>c__DisplayClass9.<GetStatus>b__8()

We're definitely loading both versions of libgit2 though:

0:039> lmvm git2_b641c00
start    end        module name
61420000 614b0000   git2_b641c00   (deferred)             
    Image path: C:\USERS\PAUL\APPDATA\LOCAL\MICROSOFT\VISUALSTUDIO\11.0\EXTENSIONS\0TX2F0B4.XSO\git2_b641c00.DLL
    Image name: git2_b641c00.DLL
    Timestamp:        Fri May 03 11:12:08 2013 (5183FDF8)
    CheckSum:         0009A76A
    ImageSize:        00090000
    File version:     0.18.0.0
    Product version:  0.18.0.0
    File flags:       0 (Mask 3F)
    File OS:          40004 NT Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0409.04e4
    ProductName:      libgit2
    InternalName:     git2.dll
    OriginalFilename: git2.dll
    ProductVersion:   0.18.0
    FileVersion:      0.18.0
    FileDescription:  libgit2 - the Git linkable library
    LegalCopyright:   Copyright (C) the libgit2 contributors. All rights reserved.
    Comments:         For more information visit http://libgit2.github.com/

0:039> lmvm git2
start    end        module name
66450000 664cb000   git2       (deferred)             
    Image path: C:\Users\Paul\AppData\Local\Microsoft\VisualStudio\11.0\Extensions\x1tplafe.sp2\git2.DLL
    Image name: git2.DLL
    Timestamp:        Fri Apr 19 14:05:43 2013 (5171B1A7)
    CheckSum:         000851FE
    ImageSize:        0007B000
    File version:     0.17.0.0
    Product version:  0.17.0.0
    File flags:       0 (Mask 3F)
    File OS:          40004 NT Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0409.04e4
    ProductName:      libgit2
    InternalName:     git2.dll
    OriginalFilename: git2.dll
    ProductVersion:   0.17.0
    FileVersion:      0.17.0
    FileDescription:  libgit2 - the Git linkable library
    LegalCopyright:   Copyright (C) the libgit2 contributors. All rights reserved.
    Comments:         For more information visit http://libgit2.github.com/

@dahlbyk
Copy link
Member
dahlbyk commented May 14, 2013

Let's supplement the FilePathMarshaler error message with the expected type/assembly/location and found type/assembly/location. It feels like wires are getting crossed somehow between FilePath types from different assemblies.

@anaisbetts
Copy link
Contributor

If anyone wants to give this a try, follow my steps on SaveAllTheTime, make sure to follow step 6 above. You also need to install the VSIX (i.e. rather than just hitting F5) because Git for VS won't be in the Experimental instance

@phkelley
Copy link
Member

Paul,

Would you mind reproing that MarshalDirectiveException again and having the debugger break when that exception is thrown (sxe clr)? I would love to take a look at a full minidump.

Thanks,
Philip

@anaisbetts
Copy link
Contributor

@phkelley Sure, though tbh it's really easy to hit, just follow the steps above and you'll have a live repro

nulltoken
nulltoken reviewed May 14, 2013
View reviewed changes
UpdateLibgit2ToSha.ps1
"@

sc -Encoding UTF8 .\Libgit2sharp\Core\NativeDllName.cs $dllNameClass
sc -Encoding UTF8 .\Lib\Libgit2sharp.dll.config $dllMap
sc -Encoding UTF8 libgit2sharp\libgit2_hash.txt $sha
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this one ASCII?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made them all ASCII. BOMs suck, man.

@nulltoken nulltoken mentioned this pull request May 14, 2013
Merged
@phkelley
Copy link
Member

Thanks Paul for providing the dump. I took a look at the issue and my initial appraisal is that it is probably a CLR bug with the construction of custom marshalers. The dump I got has the custom marshaler already created (since we are already to the point of throwing the MarshalDirectiveException), and it has the wrong MDs on it. We might entertain workarounds such as: Not using custom marshalers, or using custom marshalers that bind by string type names rather than Types. We could also see if we can get CLR to fix the issue for their next release or even for a GDR.

I will be taking a deeper look on Monday -- I am traveling this week.

@phkelley
Copy link
Member

This is confirmed as a bug in the CLR and it has been filed with them.

@dahlbyk
Copy link
Member
dahlbyk commented May 24, 2013

If managedObj as FilePath fails, we could fallback to use reflection to grab a Posix property and fail if there isn't one.

@nulltoken
Copy link
Member

@ben Manually merged. Thanks for all your help! ❤️

@nulltoken nulltoken closed this May 26, 2013
@anaisbetts
Copy link
Contributor

As I mentioned somewhere else (or maybe just in my head), we still need to fix this in libgit2sharp. Waiting on a CLR Hotfix is a non-starter, especially since non-security updates are marked as Optional in Windows Update (i.e. nobody installs them).

@nulltoken
Copy link
Member

@xpaulbettsx I agree. This one has been closed because it was scoped to making LibGit2Sharp run against a non generically named git2.dll.

Main marshaling issue is still dealt with as part of #241.

@dahlbyk dahlbyk mentioned this pull request May 27, 2013
Closed
nulltoken referenced this pull request in GitTools/GitVersion Nov 12, 2013
@SimonCropp
remove fody dependency
d980913
@bording bording mentioned this pull request Apr 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ben @nulltoken @anaisbetts @dahlbyk @phkelley
0