Skip to content
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

[6.0.100-preview.4.21178.2] Package LibGit2Sharp not working with 6.0.100-preview.4.21178.2 #50410

Closed
jiangzeng01 opened this issue Mar 30, 2021 · 6 comments · Fixed by #50655

Comments

@jiangzeng01
Copy link
Contributor

jiangzeng01 commented Mar 30, 2021

Application Name: 3 Global tools (git-istage, github-issues-cli, GitVersion.Tool)
Apps failed by package: https://github.com/libgit2/libgit2sharp
OS: Windows 10 RS5
CPU: X64.NET Build Number: main branch - 6.0.100-preview.4.21178.2
DevDiv bug: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1303009
Verify Scenarios:
1). Windows 10 RS5 X64 + .NET Core SDK build 6.0.100-preview.4.21178.2: Fail
2). Windows 10 RS5 X64 + .NET Core SDK build 6.0.100-preview.2.21158.2: Pass
3). Windows 10 RS5 X64 + .NET Core SDK build 5.0.104 : Pass
4). Windows 10 RS5 X64 + .NET Core SDK build 3.1.300 Pass

Repro steps to run

  1. Create a .net core console app
  2. Add <PackageReference Include="LibGit2Sharp" Version="0.26.2" />
  3. In Program.cs
     var p = Repository.Discover(@"PROJECT PATH WITH GIT FOLDER");
     Console.WriteLine(p);

Expected Result:
Should return git folder path for the given project

Actual Result:
Returns empty string

Findings
We think, it doesn't load native git library : git2-106a5f2.dll, it causes this issue.
.NET 6 SDK 6.0.100-preview.4.21178.2 breaks the package LibGit2Sharp and caused 3 global tools failed in AppCompat lab,

@dotnet-actwx-bot @dotnet/compat

@jeffschwMSFT jeffschwMSFT added untriaged New issue has not been triaged by the area owner area-Interop-coreclr labels Mar 30, 2021
@jiangzeng01
Copy link
Contributor Author

We verified this issue again today:
Main branch (dotnet-sdk-6.0.100-preview.4.21180.17): still failing
Preview 3 branch (dotnet-sdk-6.0.100-preview.3.21180.8): All passed
So it should be an issue which only happens in main branch.

@AaronRobinsonMSFT
Copy link
Member

/cc @jkoritzinsky @elinor-fung

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Mar 31, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 6.0.0 milestone Mar 31, 2021
@elinor-fung
Copy link
Member

LibGit2Sharp is passing a sequential layout class (GitBuf) to native, updating it, and expecting the changes to be propagated. I think this is a change in behaviour as a result of #50137.

Basic scenario is:

[StructLayout(LayoutKind.Sequential)]
internal class TestClass
{
    public IntPtr ptr;
    public UIntPtr size;
}

private static unsafe class NativeLib
{
    [DllImport(nameof(NativeLib), CallingConvention = CallingConvention.Cdecl)]
    public static extern void update_test_class(TestClass buf);
}

static void Main(string[] _)
{
    var c = new TestClass();
    NativeLib.update_test_class(c);

    // Use c.ptr / c.size, expecting that c was updated by update_test_class
}

Before: the updates made to the class in native would be received by the managed caller.
Now: either TestClass must be marked sealed or the P/Invoke must specify the OutAttribute on the parameter.

While we do have some (kind of buried) documentation pointing out the need for the OutAttribute, I fear this may have been an unintentional breaking change that may affect many use cases and should require more consideration.

Thoughts, @jkoritzinsky and @AaronRobinsonMSFT?

@jkoritzinsky
Copy link
Member

We should make the code imply default (no In or Out) as In and Out for this particular marhsaler. We can rename and modify the outImpliesIn variable in the MarshalInfo constructor to do this (It's only used in this case).

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Apr 2, 2021

Can you or @AaronRobinsonMSFT update my servicing PR with this fix (and fix master)? #50138

@jiangzeng01
Copy link
Contributor Author

This issue is verified as fixed with latest main branch build: dotnet-sdk-6.0.100-preview.4.21205.8. Most of the tests passed, and 1 test failed with new failure, see new issue #50841

@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants