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

AOT Support for Spectre.Console #1690

Merged
merged 10 commits into from
Nov 22, 2024

Conversation

phil-scott-78
Copy link
Contributor

@phil-scott-78 phil-scott-78 commented Nov 20, 2024

Made another run at AOT support. Spectre.Console itself mostly worked already, but with a few rough spots and unexpected behaviors for edge cases. The biggest issue may have been if someone was using prompts and relying on TypeConverters. @agocke did some work to use the intrinsic ones that I've included here. ExceptionHelper is marked explicitly as not working, as is the entirety of Spectre.Console.Cli.

  • Add AOT compatibility and PolySharp package support
  • Redo TypeConverterHelper based on the work by @agocke to support trimming and AOT
  • Refactor enum value retrieval to use EnumUtils for better compatibility with NetStandard 2.0 and AOT
  • Add RequiresDynamicCode attribute to exception formatter to indicate incompatibility with AOT. Adds a fallback if someone tries to use it.
  • Fix assembly name retrieval for FSharp.Core in TypeNameHelper to work better in AOT scenarios
  • Explicitly marks Spectre.Console.Cli as not trimmable and not appropriate for AOT scenarios. Additionally adds a warning to CommandApp for users who may try it.

Outstanding work would be

  • Documentation of AOT support, and the flag to revert to previous TypeHelper behavior
  • Pick apart how .NET itself handles ToString() these days in AOT and try to mimic that behavior

This replaces the previous PR #1508 and closes #1332 and #1401.

I feel this is pretty solid, and ready for review and merge. I know @Armando-CodeCafe, @BlazeFace, @antoinebj and @TheExiledCat all have express interest recently in the feature so their input would also be appreciated.


Please upvote 👍 this pull request if you are interested in it.

@Armando-CodeCafe
Copy link

This is already a great start and should already help in alot of use cases.

What im mostly surprised about is that Spectre.Console is apparently easier to get Native AOT on than Spectre.Console.Cli (im assuming thats due to reflection in the background?)

Considering most users of Spectre.Console also use the cli for their projects i would consider that the next big target. Maybe using source generators instead of reflection (again, if thats the case)

@Armando-CodeCafe
Copy link

Made another run at AOT support. Spectre.Console itself mostly worked already, but with a few rough spots and unexpected behaviors for edge cases. The biggest issue may have been if someone was using prompts and relying on TypeConverters. @agocke did some work to use the intrinsic ones that I've included here. ExceptionHelper is marked explicitly as not working, as is the entirety of Spectre.Console.Cli.

  • Add AOT compatibility and PolySharp package support
  • Redo TypeConverterHelper based on the work by @agocke to support trimming and AOT
  • Refactor enum value retrieval to use EnumUtils for better compatibility with NetStandard 2.0 and AOT
  • Add RequiresDynamicCode attribute to exception formatter to indicate incompatibility with AOT. Adds a fallback if someone tries to use it.
  • Fix assembly name retrieval for FSharp.Core in TypeNameHelper to work better in AOT scenarios
  • Explicitly marks Spectre.Console.Cli as not trimmable and not appropriate for AOT scenarios. Additionally adds a warning to CommandApp for users who may try it.

Outstanding work would be

  • Documentation of AOT support, and the flag to revert to previous TypeHelper behavior
  • Pick apart how .NET itself handles ToString() these days in AOT and try to mimic that behavior

This replaces the previous PR #1508 and closes #1332 and #1401.

I feel this is pretty solid, and ready for review and merge. I know @Armando-CodeCafe, @BlazeFace, @antoinebj and @TheExiledCat all have express interest recently in the feature so their input would also be appreciated.

When this is merged i will try it out on my projects that i was getting errors on and i will come back with my results here

@GOATS2K
Copy link

GOATS2K commented Nov 20, 2024

Very excited to try this out!

@patriksvensson
Copy link
Contributor

Very excited to merge this! 😁

@agocke
Copy link

agocke commented Nov 21, 2024

Made another run at AOT support. Spectre.Console itself mostly worked already, but with a few rough spots and unexpected behaviors for edge cases. The biggest issue may have been if someone was using prompts and relying on TypeConverters. @agocke did some work to use the intrinsic ones that I've included here. ExceptionHelper is marked explicitly as not working, as is the entirety of Spectre.Console.Cli.

I agree with this general approach. I tried really hard to get Spectre.Console.Cli to work with a source generator, but ran into too many problems that directly affected the surface area. The dependency injection approach is already really hard to make work without reflection. But even stuff like this:

app.Configure(config =>
{
    config.AddCommand<AddCommand>("add");
    config.AddCommand<CommitCommand>("commit");
    config.AddCommand<RebaseCommand>("rebase");
})

The TCommand parameter there contains properties that are supposed to be the inputs for the command parsing, but ICommand interface doesn't specify anything about them. That basically forces reflection to be used to light up the inputs.

By the time I got something kind-of working the API was just too different. It felt like a reboot. And at that point I figured I might as well try to reuse some code.

My eventual solution was to build a "CLI" format for my serde system: https://github.com/dn-vm/dnvm/tree/main/src/Serde.CmdLine. Serde is a framework for expressing serializers and deserializers and uses a source generator. The model is intended to allow swapping in different formats. CLI parsing is, in some sense, a serialization/deserialization format. So I built a format for that.

It's still very simplistic, but after I round off the edges I might create a NuGet package for it.

@TheExiledCat
Copy link

Made another run at AOT support. Spectre.Console itself mostly worked already, but with a few rough spots and unexpected behaviors for edge cases. The biggest issue may have been if someone was using prompts and relying on TypeConverters. @agocke did some work to use the intrinsic ones that I've included here. ExceptionHelper is marked explicitly as not working, as is the entirety of Spectre.Console.Cli.

I agree with this general approach. I tried really hard to get Spectre.Console.Cli to work with a source generator, but ran into too many problems that directly affected the surface area. The dependency injection approach is already really hard to make work without reflection. But even stuff like this:

app.Configure(config =>
{
    config.AddCommand<AddCommand>("add");
    config.AddCommand<CommitCommand>("commit");
    config.AddCommand<RebaseCommand>("rebase");
})

The TCommand parameter there contains properties that are supposed to be the inputs for the command parsing, but ICommand interface doesn't specify anything about them. That basically forces reflection to be used to light up the inputs.

By the time I got something kind-of working the API was just too different. It felt like a reboot. And at that point I figured I might as well try to reuse some code.

My eventual solution was to build a "CLI" format for my serde system: https://github.com/dn-vm/dnvm/tree/main/src/Serde.CmdLine. Serde is a framework for expressing serializers and deserializers and uses a source generator. The model is intended to allow swapping in different formats. CLI parsing is, in some sense, a serialization/deserialization format. So I built a format for that.

It's still very simplistic, but after I round off the edges I might create a NuGet package for it.

This would be insanely cool if it would be able to work, however i think there would be some kind of way to do the thing in your example using for example roslyn source gens by grabbing the attributes of the Command specifying the options and settings and then the roslyn source gen would have to add code to the build manually setting the different options to their
Lets say the settings have a property called Port "--port "

The roslyn source gen would have to generate some kind of line and add it to the source like:

`Port = args.ParseThePortAndDoTheThings("--port ")

So i dont think we would need a external lib for this, using some well defined roslyn generators these commands and settings can be set using some kind of functions injected at build time that contain all of the different properties and their respective cli strings

@phil-scott-78
Copy link
Contributor Author

phil-scott-78 commented Nov 21, 2024

I think at the end of the day to do AOT in Spectre.Console.Cli would either take considerable effort or a change in the API. Between DI, TypeConverters, MultiMap<>, dynamically creating arrays and even some magic in there for automatically instantiating things like DirectoryInfo properties, there are just so many edge cases that are deep within the codebase that we'd be in a situation where we'd never know if any change was going to break AOT and/or we'd be in the weeds managing System.Diagnostics.CodeAnalysis attributes with every new update.

Additionally, a while back, we split the Cli and Console app with the knowledge there were more "modern" libraries like System.CommandLine that integrate well with Spectre.Console that having the benefit of being developed after the introduction of things like assembly trimming have support designed in.

I do think a third party that wanted to generate a source generator that scans for Settings and dumps a ton of DynamicDependency attributes for the settings and their properties might get people limping along, but I'm not sure I'd ever feel comfortable including it in the official project. I certainly wouldn't love asking the other maintainers to have to own it either.

@TheExiledCat
Copy link

I think at the end of the day to do AOT in Spectre.Console.Cli would either take considerable effort or a change in the API. Between DI, TypeConverters, MultiMap<>, dynamically creating arrays and even some magic in there for automatically instantiating things like DirectoryInfo properties, there are just so many edge cases that are deep within the codebase that we'd be in a situation where we'd never know if any change was going to break AOT and/or we'd be in the weeds managing System.Diagnostics.CodeAnalysis attributes with every new update.

Additionally, a while back, we split the Cli and Console app with the knowledge there were more "modern" libraries like System.CommandLine that integrate well with Spectre.Console that having the benefit of being developed after the introduction of things like assembly trimming have support designed in.

I do think a third party that wanted to generate a source generator that scans for Settings and dumps a ton of DynamicDependency attributes for the settings and their properties might get people limping along, but I'm not sure I'd ever feel comfortable including it in the official project. I certainly wouldn't love asking the other maintainers to have to own it either.

Thats fair, so it seems this one is just gonna be: who wants to either put an unrealistic amount of effort to change the current Codebase or just rewrite it from scratch to native aot, so the options arent great here i guess.

@patriksvensson
Copy link
Contributor

patriksvensson commented Nov 21, 2024

Once AOT for Spectre.Console is in, I will take a look at generating the command tree using source generators instead of doing it by reflection. I have some ideas.

@phil-scott-78
Copy link
Contributor Author

Redid the ExceptionFormatter code. The more I thought about it, it's not like the default .NET behavior is all that great anyways. So now it still uses the nicer format for printing the exception and message, then falls back to the StackFrame ToString() method to get as much info as we can at runtime.

While I was in there I went ahead and added support for Spectre.Console.Json and Spectre.Console.ImageSharp, and I even added a blurb in the documentation's best practice section.

@agocke
Copy link

agocke commented Nov 22, 2024

FYI I’m totally willing to help anyone who wants to dive in the rabbit hole of writing a source generator. I just hit a wall and happened to have an alternative that I could use to unblock my app. FYI I’ve been using the rest of spectre console in dnvm as aot and it has worked great.

@phil-scott-78
Copy link
Contributor Author

I have nothing really to add here anymore. Pending a review, I say ready for merge.

@patriksvensson
Copy link
Contributor

@phil-scott-78 I will be taking a look now. Not sure I will understand everything though 😁

@phil-scott-78
Copy link
Contributor Author

Anything needing clarification or more comments to describe what's happening, I'm happy to expand!

Copy link
Contributor

@patriksvensson patriksvensson left a comment

Choose a reason for hiding this comment

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

It looks good to me. A question, though: Can we remove reliance on the other polyfill packages now that PolySharp has been introduced?

Copy link
Contributor

@patriksvensson patriksvensson left a comment

Choose a reason for hiding this comment

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

It looks good to me. A question, though: Can we remove reliance on the other polyfill packages now that PolySharp has been introduced?

@phil-scott-78
Copy link
Contributor Author

Good question. I bet we could. Want me to try in this pr or a new one?

@patriksvensson
Copy link
Contributor

@phil-scott-78 We can try it in a new PR.

@patriksvensson patriksvensson merged commit b2689be into spectreconsole:main Nov 22, 2024
3 checks passed
@patriksvensson
Copy link
Contributor

Merged it!
Awesome work @phil-scott-78! 🎉

@phil-scott-78 phil-scott-78 deleted the aot-yet-again branch December 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Progress 👨‍💻
Development

Successfully merging this pull request may close these issues.

AOT Support
7 participants