[BUG]: Octokit.net is not trim compatible

hach-que commented 1 year ago

What happened?

When you build a .NET executable with trimming enabled, the linker will produce trim warnings about Octokit because it is not trim compatible:

C:\Users\jrhod\.nuget\packages\octokit\6.2.1\lib\netstandard2.0\Octokit.dll : error IL2104: Assembly 'Octokit' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries [C:\Work\unreal-engine-tool\UET\uet\uet.csproj]
C:\Program Files\dotnet\sdk\7.0.302\Sdks\Microsoft.NET.ILLink.Tasks\build\Microsoft.NET.ILLink.targets(86,5): error NETSDK1144: Optimizing assemblies for size failed. Optimization can be disabled by setting the PublishTrimmed property to false. [C:\Work\unreal-engine-
tool\UET\uet\uet.csproj]

To fix this, Octokit needs to add the following properties to the C# project files:

  <PropertyGroup>
    <IsTrimmable>true</IsTrimmable>
    <EnableTrimAnalyzer>true</EnableTrimAnalyzer>
    <EnableSingleFileAnalyzer>true</EnableSingleFileAnalyzer>
  </PropertyGroup>

Versions

6.2.1

Relevant log output

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

IEvangelist commented 9 months ago

I'd like to work on this, leaving a comment to make everyone aware that I'm going to prepare a pull request. It might be rather large, as it will take a lot of effort to enable the library to be AOT trimmable.

We'll need to multitarget, no longer just a TFM of netstandard2.0. I'll add net6.0, net7.0, and net8.0.
I'll need to clear most build warnings, so that it's easier to diagnose and fix trim-related warnings.
I'm not sure what else will need to be done to enable this, but it will likely require added preprocessor directives for AOT versions, with decorated attributes such as DynamicallyAccessedMembers.

IEvangelist commented 9 months ago

I hit a snag, I'm realizing that the SimpleJson and SimpleJsonSerializer classes are not trimmable. I think we should expose a System.Text.Json implementation of the IJsonSerializer when not netstandard2.0.

Some problematic APIs:

This will allow us to support trimming when building for net6.0, net7.0, and net8.0, but it will likely require a lot of changes.

IEvangelist commented 9 months ago

My use case for wanting this library to be trimmable, is that I plan on using it in several GitHub Action .NET container apps, where startup time is greatly improved when using AOT. This is vital to the success of the adoption of these apps.

Right, so after digging into this - it's going to take a huge amount of time / effort. Here's my findings, summarized as succinctly as possible:

I assume we don't want to remove support for netstandard2.0, so that would mean that we will need to multitarget in Octokit and optionally Octokit.Reactive.
The IJsonSerializer abstraction is nice and simple, but there are hardcoded dependencies where the SimpleJsonSerializer are used - in fact, the SimpleJsonSerializer is the only implementation (even in tests). Maybe we should drop it, or embrace it and add perhaps a System.Text.Json (STJ) implementation?

This would allow us to have support for .NET 6+ (meaning Native AOT, trimmable). But only when targeting .NET 6+. STJ isn't supported in netstandard2.0.
The IConnection, IRequest, and IApiConnection APIs are work with generic returns, but object based parameters. We'd need to update many call sites to instead accept generic-type parameters (think TArg, or TBody or TData, etc.), and they're corresponding JsonTypeInfo<TArg> typo info. This would be used by STJ serializer to serialize and deserialize the types.
All request and response models that are serialized will need to have a partial subclass of JsonSerializerContext and be decorated with JsonSerialable attribute to source-generate the type infos, and those will have to be provided to the internal workings such as the Connection implementation.

I was describing this situation to a co-worker and they asked what the alternative is to this approach, I don't think there's much that can be done otherwise besides a complete rewrite - which seems like way more work.

I do have a branch that I've been working on, but I would like to solicit feedback before continuing work: https://github.com/octokit/octokit.net/compare/main...IEvangelist:octokit.net:trimming-aot

martincostello commented 9 months ago

+1 to the ask for support for native AoT here (and in octokit/webhooks.net) - I have a number of apps I use both libraries in, but those are currently blocked from using AoT because I use parts of ASP.NET Core that aren't yet supported, such as Razor Pages, so Octokit also not supporting is is moot for me...for now.

I will be looking to enable AoT in those apps as soon as that blocker goes away (.NET 9? 🤞), so knowing that Octokit supports native AoT in advance of that would be most welcome.

nickfloyd commented 9 months ago

Hey Friends ❤.

This is such a good discussion. Thanks @hach-que for kicking it off. Just as y'all know / discovered we have some old patterns, implementations, and code (that use reflection specifically) that make converting octokit.net to be able to benefit from AoT almost impossible.

Bad news out of the way, here's the good news... 🎉

We are in the middle of a seriously exciting effort where we are using the incredible work that our Microsoft friends have done (Kiota) and are leaning heavily into generating our SDKs - We alluded to this over a year ago and have continued the discussion here and here).

So what does this mean?

We'll get AoT support right out of the box with the newly generated dotnet-sdk.

What else does this mean?

We'll get close to 100% GitHub REST API coverage, have an SDK that supports REST API versioning (lowers the risk of breaking changes), support more Auth patterns, and begin to unlock a host of other things/patterns that we were previously not able to do. Oh and we'll be supporting more languages eventually

When is this happening?

Hopefully sooner than later. We've been working through some prototyping and are fairly close on getting our thoughts down as code. The goal is to get this things as "right" as we can so we are being measured about how we approach this so that it will be beneficial to the community.

Please let @kfcampbell or me (@nickfloyd) know if y'all have any questions or want more information.

terrajobst commented 9 months ago

@nickfloyd

Does this mean you're designing the client side SDK for GitHub?

We (Microsoft Open Source Program Office and the .NET teams) are heavy users of the GitHub API (Ocotkit.NET, including the GraphQL version). If you're in the middle of redesigning it we'd love to give you some feedback. Happy to do a call or if you prefer we can give you a write up, whatever is easier.

nickfloyd commented 9 months ago

@terrajobst,

We'd love some feedback and a discussion! If you (or anyone else on this thread) get a chance please jot down your thoughts on this discussion thread and I'll reach out to you via teams to see if we can hop on a call depending on your availability.

Thank you for making time for this; we need feedback, ideas, and insight if we're going to deliver something that the community will actually need/want.

JamieMagee commented 9 months ago

There was a relevant blog post published yesterday: https://devblogs.microsoft.com/dotnet/creating-aot-compatible-libraries/

IEvangelist commented 9 months ago

There was a relevant blog post published yesterday: https://devblogs.microsoft.com/dotnet/creating-aot-compatible-libraries/

Yes, @eerhardt and I were talking about this. I think the decision has been made with this project to source generate the clients instead of taking the time/effort to trim here. But I could be wrong.

nickfloyd commented 9 months ago

That is correct. Instead of trying to invest heavily in converting Octokit.net we are beginning the designs and ideation on a generated SDK (using Kiota) that will provide features like trimming out of the box.

We're not quite ready to EoL Octokit.net or anything just yet but we feel with y'alls help and input we're going to be able to create something that is not only sustainable but a thriving tool that will help us all build the next generation of tooling. ❤

Please consider getting involved. Y'all are getting a sneak peek of things to come - we'll be dropping a blog post with details and a call to action and more details on how to get involved if you want to - we know we'll need everyone's input to get this moving in the right direction so I am really hoping y'all decide to join us on this new adventure! 🗻

Side note: We are so grateful for each of you! The fact that you care so much about the work you do and the community you're in is super inspiring. Thank you all for what you do for our industry and your users.

github-actions[bot] commented 1 day ago

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

octokit / octokit.net