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

Please consider adding AOT support for .Diff.Compare<T>() #2082

Open
darkthread opened this issue Mar 8, 2024 · 19 comments
Open

Please consider adding AOT support for .Diff.Compare<T>() #2082

darkthread opened this issue Mar 8, 2024 · 19 comments

Comments

@darkthread
Copy link

When utilizing libgit2sharp in combination with the .NET 8 PublishAot feature, all repository creation, file staging and committing operations are successful, however, the git diff function throws a NullReferenceException.

Is there a way to get this working under AOT?

Steps To Reproduce:

  1. Create a .NET 8 console project
    dotnet new console -o aot-test
    cd aot-test
  2. Adjust aot-test.csproj to include libgit2sharp reference and activate PublishAot feature
    <Project Sdk="Microsoft.NET.Sdk">
    
      <PropertyGroup>
    	<OutputType>Exe</OutputType>
    	<TargetFramework>net8.0</TargetFramework>
    	<RootNamespace>aot_test</RootNamespace>
    	<ImplicitUsings>enable</ImplicitUsings>
    	<Nullable>enable</Nullable>
    	<PublishAot>true</PublishAot>
      </PropertyGroup>
    
      <ItemGroup>
    	<PackageReference Include="libgit2sharp" Version="0.29.0" />
      </ItemGroup>
    
    </Project>
  3. Insert the following code into Progmram.cs:
using LibGit2Sharp;

var path = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(path);
Repository.Init(path, false);
var userName = "Tester";
var email = "tester@mail.net";
Func<Signature> getSignature = () => new Signature(userName, email, DateTimeOffset.Now);
using (var repo = new Repository(path))
{
    var fileName = "test.txt";
    var filePath = Path.Combine(path, fileName);
    File.WriteAllText(filePath, "Hello, World!");
    Commands.Stage(repo, fileName);
    var commit1 = repo.Commit("Initial commit", getSignature(), getSignature());
    File.AppendAllText(filePath, "\nAppended Line");
    Commands.Stage(repo, fileName);
    var commit2 = repo.Commit("Second commit", getSignature(), getSignature());
    Console.WriteLine($"Commits Count {repo.Commits.Count()}");
    var changes = repo.Diff.Compare<Patch>(commit1.Tree, commit2.Tree);
    Console.WriteLine($"Diff Changes Count = {changes.Count()}");
}
  1. Use dotnet run for testing, it will display:
Commits Count 2
Diff Changes Count = 1
  1. Use dotnet publish, .\bin\Release\net8.0\win-x64\publish\aot-test.exe to test the native AOT version, it throws an exception on repo.Diff.Compare<Patch>():
Commits Count 2
Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
   at aot-test!<BaseAddress>+0x19af4c
   at aot-test!<BaseAddress>+0x199437

Current Behavior:

The native AOT functions for repository creation, file staging, and committing operations are functional, but the git diff function is not.

Anticipated Behavior:

Please consider adding AOT support for the git diff API.

Version of LibGit2Sharp (release number or SHA1)

0.29.0

Operating system(s) tested; .NET runtime tested

  • OS: Windows 11
  • .NET: 8.0.100
@igiona
Copy link

igiona commented Mar 8, 2024

Interesting that you're looking into AOT these days, since I'm on it as well 😄

Unfortunately, not all operations are successful and it's not only the git-diff that doesn't work.
Also the simple Discover functionality is broken:

            Console.WriteLine("Hello, World!");

            var repo = Repository.Discover(@"C:\git\a-repo");

            if (repo == null)
            {
                Console.WriteLine("Doh! Repo not found");
            }
            else
            {
                Console.WriteLine("Found repo at {0}", repo);
            }

I tried to make the library AOTable igiona#1 but it doesn't seem to help solving my problem.
Later I will try to see if it does solves yours.

The Discover issue boils down to a marshalling problem with the native library in:

        [DllImport(libgit2, CallingConvention = CallingConvention.Cdecl)]
        internal static extern int git_repository_discover(
            GitBuf buf,
            [MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictFilePathMarshaler))] FilePath start_path,
            [MarshalAs(UnmanagedType.Bool)] bool across_fs,
            [MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictFilePathMarshaler))] FilePath ceiling_dirs);

basically buf returns "null" at some point...

@ethomson
Copy link
Member

ethomson commented Mar 8, 2024

Repository.Discover returns null when there was no repository at that path. It works when compiled to a normal assembly, but not when compiled to AOT?

Does AOT do something funny with the filesystem?

@darkthread
Copy link
Author

@ethomson It's interesting to me that add, commit and log can work in AOT. I have no idea why diff throws an exception.

@igiona
Copy link

igiona commented Mar 8, 2024

Repository.Discover returns null when there was no repository at that path. It works when compiled to a normal assembly, but not when compiled to AOT?

Does AOT do something funny with the filesystem?

Correct, it just doesn't work in AOT.
To narrow down the problem even further, I went on a even simpler call: git_config_find_global .

I exposed it nastily to my Program.cs:

Configuration.cs

public static string TestMe() => Proxy.git_config_find_global()?.ToString() ?? "Was null";

Program.cs

   Console.WriteLine("=> {0}", Configuration.TestMe());

=> Was null in AOT
my-user-home\.gitconfig non-AOT

Also this call accesses the file-system, weather this is the relation though is hard to say for me.
What's interesting is that the native call returns actually a valid result (0=Ok), but a "null" buffer pointer:

I added some debugging information in ConvertPath

        private static FilePath ConvertPath(Func<GitBuf, int> pathRetriever)
        {
            using (var buf = new GitBuf())
            {
                int result = pathRetriever(buf);
                Console.WriteLine($"=> result {result}");
                Console.WriteLine($"=> ptr {buf.ptr:x}");
                if (result == (int)GitErrorCode.NotFound)
                {
                    return null;
                }

                Ensure.ZeroResult(result);
                return LaxFilePathMarshaler.FromNative(buf.ptr);
            }
        }

AOT

=> result 0
=> ptr 0

@igiona
Copy link

igiona commented Mar 8, 2024

@ethomson It's interesting to me that add, commit and log can work in AOT. I have no idea why diff throws an exception.

I still get the null exception even in the AOT "friendly" branch.

@darkthread
Copy link
Author

I guess I found the reason why AOT version doesn't work. The problem is more complicated than I thought. According to the documentation:

The Native AOT application model addresses issues with dynamic code generation by precompiling all code ahead of time directly into native code.
The P/Invoke source generator, included with the .NET 7 SDK and enabled by default, looks for LibraryImportAttribute on a static and partial method to trigger compile-time source generation of marshalling code, removing the need for the generation of an IL stub at run time and allowing the P/Invoke to be inlined.

In other words, take 'git_config_find_global' as an example, we need to change [DllImport] part to internal static partial int git_config_find_global, and then redefine a set of [LibraryImport] internal static partial int git_config_find_global. Also, we need to define [CustomMarshaller(typeof(GitBuf), MarshalMode.Default, typeof(GitBufMarshaller))] internal static unsafe class GitBufMarshaller to handle parameters marshaing.

I would like to give it a try, but dealing with Unmanaged code is beyond my capabilities.

@bording
Copy link
Member

bording commented Mar 11, 2024

I did take a look at this a bit this weekend, and I'm not sure that the issue is solely a DllImport vs. LibraryImport thing. There is enough evidence around that at least some forms of DllImport should just work as part of AOT. For example, this article exists and doesn't mention needing to use the source-generated P/Invokes.

It also seems a bit strange to me that there are analyzers that that point out other problematic code for AOT, but don't flag DllImport as a problem. Seems like they would if that was required.

That being said, I do think the source of the problem is related to the non-trivial uses of DllImport in this library that you don't see in a lot of other interop scenarios. Moving to LibraryImport might address that, but I do recall that the last time I looked into using them, they didn't support all the things required by the existing DllImport calls in here.

It would be pretty easy to get LibGit2Sharp fully compatible with trimming and single-file deployment, but getting all the way to AOT is likely going to be a lot of tricky work, if it's possible at all currently.

@bording
Copy link
Member

bording commented Mar 12, 2024

Found a couple more issues on the dotnet repo that likely explain a bit more what's going on here. Currently it looks like warnings for AOT-problematic P/Invokes via DllImport are incomplete: dotnet/runtime#74697

It also sounds like one benefit of moving to the source generator imports would be that you would actually get warnings for P/Invokes that aren't compatible with AOT.

However, even if those places were properly identified, it's not clear to me yet if there would even be a way to change LibGit2Sharp to accommodate the AOT limitations. It's quite possible that a change on the libgit2 side would be required.

@ethomson
Copy link
Member

What would we need to do on the libgit2 side?

@bording
Copy link
Member

bording commented Mar 12, 2024

What would we need to do on the libgit2 side?

I'm just theorizing here, but if it turns out there was a libgit2 API pattern that there was no way to adjust the P/Invoke for to make it compatible with AOT, then the only way to solve it currently would be to modify the libgit2 API. Otherwise, we'd be waiting for Microsoft to change something in their AOT stack to make it work.

I don't yet know if there is anything like that, though. It may turn out that every DllImport can be changed to work with AOT. The main problem at this point is there is nothing pointing to the problematic ones or warnings to tell me what part of it is not compatible with AOT.

@ethomson
Copy link
Member

I'm just theorizing here, but if it turns out there was a libgit2 API pattern that there was no way to adjust the P/Invoke for to make it compatible with AOT, then the only way to solve it currently would be to modify the libgit2 API.

Got it. Happy to work on this if we need to. I think that we could also figure something out here where we build a shim over libgit2 to not break API compatibility but mutate it into something AOT can handle. 🤷

@igiona
Copy link

igiona commented Mar 13, 2024

Having this compatible with AOT would be a huge benefit for tools using libgit2sharp, like GitVersion.
Finally there would be a tool which can handle semantic versioning without hassle and dependency to the .NET runtime.

I'm happy to help here, but source generators and unmanaged code are a new field for me

@bording
Copy link
Member

bording commented Mar 13, 2024

Finally there would be a tool which can handle semantic versioning without hassle and dependency to the .NET runtime.

That could already be achieved through publishing a trimmed self-contained application. AOT isn't required for that.

@igiona
Copy link

igiona commented Mar 13, 2024

Finally there would be a tool which can handle semantic versioning without hassle and dependency to the .NET runtime.

That could already be achieved through publishing a trimmed self-contained application. AOT isn't required for that.

That's an interesting approach indeed, I gave it a quick try on GitVersion targeting linux-x64:

self-contained, master untrimmed : 75.5 MB (224 files)
self-contained, master trimmed: 26.8 MB (86 files)
self-contained, MakeLibTrimFriendly trimmed: 28.1 MB (95files)
AOT, CliAot : 20.0MB (2 files => I removed the .pdb)

I tested it by simply run the binary with default settings on a "normal" repo.

A part of the startup time which is remarkably faster on AOT, self-contained + trimming seems like a valuable and somewhat easier solution.
Although addressing all the trimming warnings will likely be quite a hassle as well...

@bording
Copy link
Member

bording commented Mar 13, 2024

Although addressing all the trimming warnings will likely bit quite a hassle as well...

I've already fixed them all locally while investigating the AOT stuff this weekend. They were actually nothing too bad. I'll look into pushing them up and getting them out soon.

A part of the startup time which is remarkably faster on AOT

Yes, startup time is one thing that AOT would improve.

@igiona
Copy link

igiona commented Mar 13, 2024

Although addressing all the trimming warnings will likely bit quite a hassle as well...

I've already fixed them all locally while investigating the AOT stuff this weekend. They were actually nothing too bad. I'll look into pushing them up and getting them out soon.

A part of the startup time which is remarkably faster on AOT

Yes, startup time is one thing that AOT would improve.

That's nice!
I guess you refer libgit2sharp only? My comment was more on the GitVersion tool side :)

@bording
Copy link
Member

bording commented Mar 13, 2024

Yes, I meant for LibGit2Sharp, which is all that is relevant here. 😄

But yes, all trimming errors would need to be addressed for whatever program you're trying to trim. AOT uses trimming as well, so the first prerequisite is to address all trimming problems.

@bording
Copy link
Member

bording commented Mar 19, 2024

I just released LibGit2Sharp 0.30.0, which includes #2084, so it should be fully compatible with trimming and single file publishing now.

I also started looking into what is involved in converting from DllImport to LibraryImport, and yeah that's going to take some work to get done.

For starters, LibraryImport is only available on .NET 7+, so that's going to mean I'll need to add a net8.0 target to the project to be able to use it. That also means the DllImport version of the code has to stay to keep supporting net472 and net6.0, which isn't ideal.

In some ways, switching to LibraryImport simplifies things, for example how string marshalling is done. There is a now a UTF-8 string marshalling class built in, so I can use that instead of needing a custom marshaller.

However, there are some new requirements around the types involved in the P/Invoke definitions, and I suspect those are what are tripping up using AOT right now.

Effectively, a lot of the existing interop types defined in LibGit2Sharp aren't blittable and require runtime marshalling to actually work. LibraryImport requires everything to either be a blittable type, or a struct made up of those types since the runtime marshalling doesn't exist for them.

Lots of the interop types are defined as classes right now, and those need to be redefined as structs (along with other changes) to be able to work. However, making those changes has impacts on the higher layers of the code that also have to be understood and adjusted.

I'm going to keep working on it, but no promises as to when I'll be able get it all done!

For anyone interested in the details of the changes, I've pushed up my spike branch: https://github.com/libgit2/libgit2sharp/tree/libraryimport

MIRIMIRIM added a commit to Nekomoekissaten-SUB/Mobsub that referenced this issue Mar 21, 2024
@MIRIMIRIM
Copy link

For anyone interested in the details of the changes, I've pushed up my spike branch: https://github.com/libgit2/libgit2sharp/tree/libraryimport

Thank you for your dedication to AOT support. I’m wondering if you plan to provide static libraries for multiple platforms that support static linking in native AOT, akin to the dynamic libraries currently available.

MIRIMIRIM added a commit to Nekomoekissaten-SUB/Mobsub that referenced this issue Apr 4, 2024
* SubtitleParse/AssEvent: add Read(); Write() use stringbuilder

* Ikkoku: extract NotZhConvert method

* Ikkoku: init simple GitMergeDiff

but can’t aot, wait libgit2/libgit2sharp#2082

* GitMergeDiff: fix paths and not use aot

* IncludeNativeLibraries when PublishSingleFile

* Ikkoku/Clean: trimend when process event

It is a regression bug caused by 244bb2b.
I used to trim space chars at the end of the event line when parse ass, but now I have moved it to the ikkoku/clean

* Update ikkoku-dotnet-ci.yml

* Update ikkoku-dotnet-ci.yml

* Ikkoku/Clean: fix RemoveEndSpace wrong return

* SubtitleParse: organize codes

* SubtitleParse: move to src folder

* SubtitleParse: update package info

* SubtitleParse: fix script info parse

* Test: update

* Ikkoku: split and adjust to adapt SubtitleParse

* Ikkoku: move files

* Ikkoku: split Program

* Ikkoku: rename GitMergeDiff

* Ikkoku: merge add subcommand base-diff

* Ikkoku/clean: add deleteFanhuaji (enable in preset More)

* Ikkoku: disable mege/base-diff when NativeAOT

* bump version: Ikkoku 0.3.2; SubtitleParse 0.3.0

* gha: update ikkoku-dotnet-ci.yml
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

No branches or pull requests

5 participants