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 #1332

Closed
lukewis opened this issue Oct 3, 2023 · 22 comments · Fixed by #1690
Closed

AOT Support #1332

lukewis opened this issue Oct 3, 2023 · 22 comments · Fixed by #1690

Comments

@lukewis
Copy link

lukewis commented Oct 3, 2023

Is your feature request related to a problem? Please describe.
This library actually works in the context of native code generation (AOT), but it produces a compiler warning (which makes me suspect there are some features in this library that would not work from an AOT perspective.

Describe the solution you'd like
Would love to see this library run cleanly in an AOT build (AOT seems like a primary candidate for a library like this to create multi-platform commandline tools that do not rely on the dotnet framework being installed.

Describe alternatives you've considered
None

Additional context

.nuget\packages\spectre.console\0.47.0\lib\net7.0\Spectre.Console.dll : warning IL2104: Assembly
'Spectre.Console' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries [C:\src\alpsdev\alpsc
li.csproj]

Please upvote 👍 this issue if you are interested in it.

@borrrden
Copy link

FYI I used the following to get this project to work with AOT:

Create some file (e.g. MyRoots.xml) with the following content (substitute MyAssembly with your assembly name):

<linker>
    <assembly fullname="MyAssembly" />
    <assembly fullname="Spectre.Console.Cli">
        <type fullname="Spectre.Console.Cli.ExplainCommand" />
        <type fullname="Spectre.Console.Cli.XmlDocCommand" />
        <type fullname="Spectre.Console.Cli.VersionCommand" />
    </assembly>
</linker>

This makes sure that all the reflected commands needed don't get trimmed out. Then make use of it in your csproj:

<ItemGroup>
  <TrimmerRootDescriptor Include="MyRoots.xml" />
</ItemGroup>

@lukewis
Copy link
Author

lukewis commented Nov 27, 2023

Hmm.... things seemed to be working for me. I was just interested in cleaning up the warnings, but your answer now makes me wonder if some things are broken that i didn't catch. I'll have to take another look

@borrrden
Copy link

The project I am working on is here on GitHub so if you are interested in seeing if you see what I see then I can push what I discovered up into the repo (https://github.com/borrrden/Spectre.Net). Coincidentally the project started as a port of a separate unrelated project also called Spectre and then I remembered that this repo existed and thought it was poetic.

@borrrden
Copy link

There are still a few more issues I am having with AOT:

  • Using an enum as a command setting fails and throws the not very helpful error Error: EnumConverter cannot convert from System.Int32.. Workaround is to assign it a custom TypeConverter that can convert it from int.

  • Default value shows integral value rather than string value. Workaround unknown, so I disabled default value display in the help.

@andwi
Copy link

andwi commented Nov 29, 2023

An alternative to the xml file mentioned by @borrrden that worked for me is to use the DynamicDependency attribute in your code. For example I have a command called DiffCommand and my entrypoint looks like this

[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(DiffCommand))]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, typeof(DiffCommandSettings))]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, "Spectre.Console.Cli.ExplainCommand", "Spectre.Console.Cli")]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, "Spectre.Console.Cli.VersionCommand", "Spectre.Console.Cli")]
[DynamicDependency(DynamicallyAccessedMemberTypes.All, "Spectre.Console.Cli.XmlDocCommand", "Spectre.Console.Cli")]
private static int Main(string[] args)
{
}

@borrrden
Copy link

borrrden commented Dec 1, 2023

@andwi Would you happen to know anything about my enum situation? 😄

@andwi
Copy link

andwi commented Dec 1, 2023

@andwi Would you happen to know anything about my enum situation? 😄

Sorry no, my application has not used any enum values so I have not encountered that

fvn-elmy added a commit to fvn-elmy/fsharp-spectre-console-template that referenced this issue Jan 16, 2024
build using `dotnet publish -r linux-amd64 -c Release`
test with `bin/Release/net8.0/linux-x64/fsharp-spectre-console-template greet -n John`

based on spectreconsole/spectre.console#1332 (comment)
@Mischala
Copy link

Mischala commented Feb 25, 2024

I use CommandApp and AppSettings in my application.
I can publish with:

<OutputType>Exe</OutputType>
<PublishAot>true</PublishAot>

and it seems to work just fine, however, I get the following warnings.
Would be nice to mark any Dynamic access in the library.

ILC : Trim analysis warning IL2092: Libbyget.Infrastructure.TypeRegistrar.Register(Type,Type): 'DynamicallyAccessedMemb
erTypes' in 'DynamicallyAccessedMembersAttribute' on the parameter 'implementation' of method 'Libbyget.Infrastructure.
TypeRegistrar.Register(Type,Type)' don't match overridden parameter 'implementation' of method 'Spectre.Console.Cli.ITy
peRegistrar.Register(Type,Type)'. All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage
. [redacted
C:\Users\\.nuget\packages\spectre.console.cli\0.48.0\lib\net8.0\Spectre.Console.Cli.dll : warning IL2104: Assembly
 'Spectre.Console.Cli' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries [C:\User
s\\Dev\LibbyGet\Libbyget\LibbyGet.csproj]
C:\Users\.nuget\packages\spectre.console.cli\0.48.0\lib\net8.0\Spectre.Console.Cli.dll : warning IL3053: Assembly
 'Spectre.Console.Cli' produced AOT analysis warnings. [redacted
/_/src/Spectre.Console.Cli/Internal/Modelling/CommandModel.cs(48): warning IL3000: Spectre.Console.Cli.CommandModel.Get
ApplicationFile(): 'System.Reflection.Assembly.Location.get' always returns an empty string for assemblies embedded in
a single-file app. If the path to the app directory is needed, consider calling 'System.AppContext.BaseDirectory'. [redacted]
C:\Users\\.nuget\packages\spectre.console\0.48.0\lib\net8.0\Spectre.Console.dll : warning IL2104: Assembly 'Spectr
e.Console' produced trim warnings. For more information see https://aka.ms/dotnet-illink/libraries [redacted]

@TheExiledCat
Copy link

TheExiledCat commented Sep 15, 2024

Anyone else get the error

Error: Could not find a constructor for 'Spectre.Console.Cli.ExplainCommand+Settings'.

not only do i get trim warnings but the app commands dont want to run because this explain command (which im assuming is the builtin command that polls your custom commands) wont work.

Ps. works fine without AOT. so im assuming something is going wrong with the assemblies loaded

@antoinebj
Copy link

FYI in the base Spectre.Console package, using TextPrompt or AnsiConsole.WriteException() triggers trim warnings. It would be nice to make those disappear. Console applications are prime candidates for AOT, they're usually expected to be lightweight distributables. It's no wonder that this is the top issue.

@patriksvensson
Copy link
Contributor

Yup. Just waiting for someone who wants it enough to send a PR 🙂

@Armando-CodeCafe
Copy link

Yup. Just waiting for someone who wants it enough to send a PR 🙂

Hopefully someone with some AOT library knowledge can hop on this quickly!

@BlazeFace
Copy link
Contributor

@patriksvensson I can take this issue, I've done something similar in the past.

@Armando-CodeCafe
Copy link

@patriksvensson I can take this issue, I've done something similar in the past.

You're a legend mate, good luck. Ill be watching closely

@BlazeFace
Copy link
Contributor

@patriksvensson For most of the updates we would need to use tooling added in .net7. It also will likely require #if NET7_0_OR_GREATER etc in order to maintain support for AOT for .NET 7,8,9 while also supporting netstandard and net6.

Would this work for you?

For example

#if NET7_0_OR_GREATER
    public static List<string> GetMarkupNames(Decoration decoration)
    {
        var result = new List<string>();
        Enum.GetValues<Decoration>()
            .Cast<Decoration>()
            .Where(flag => (decoration & flag) != 0)
            .ForEach(flag =>
            {
                if (flag != Decoration.None && _reverseLookup.TryGetValue(flag, out var name))
                {
                    result.Add(name);
                }
            });

        return result;
}
#else
    public static List<string> GetMarkupNames(Decoration decoration)
    {
        var result = new List<string>();
        Enum.GetValues(typeof(Decoration))
            .Cast<Decoration>()
            .Where(flag => (decoration & flag) != 0)
            .ForEach(flag =>
            {
                if (flag != Decoration.None && _reverseLookup.TryGetValue(flag, out var name))
                {
                    result.Add(name);
                }
            });

        return result;
    }
#endif

@patriksvensson
Copy link
Contributor

No, things like that would be a pain to maintain. NET7 support could be removed though since it's end of life.

@antoinebj
Copy link

@BlazeFace You could make more targeted changes:

    public static List<string> GetMarkupNames(Decoration decoration)
    {
        var result = new List<string>();

        var decorationValues =
#if NET5_0_OR_GREATER
            Enum.GetValues<Decoration>();
#else
            Enum.GetValues(typeof(Decoration)).Cast<Decoration>();
#endif

        decorationValues
            .Where(flag => (decoration & flag) != 0)
            .ForEach(flag =>
            {
                if (flag != Decoration.None && _reverseLookup.TryGetValue(flag, out var name))
                {
                    result.Add(name);
                }
            });

        return result;
    }

@phil-scott-78
Copy link
Contributor

Take a look at this PR of a WIP to help get you started. Myself with a TON of input from @agocke started down this path, but all roads still end ina quagmire of TypeConverter. It's a large undertaking.

#1508

@BlazeFace
Copy link
Contributor

@phil-scott-78 Thanks, that helps a lot!

@phil-scott-78
Copy link
Contributor

I ran into a brick wall of responsibilities that was a large part of the reason that PR languished, but the reality is I think to do this in a manner that would be acceptable to users and maintainers is gonna involve a major rewrite after the 1.0 release with some breaking changes along with a source generator to tie the the user code together.

I think it's a very worthy undertaking, but here be dragons

@BlazeFace
Copy link
Contributor

@phil-scott-78 Agreed, after looking over the thread I see exactly what you are mentioning. Very happy to help! Would we be ok with just the console project AOT compatible while Spectre CLI support coming later?

@Armando-CodeCafe
Copy link

Armando-CodeCafe commented Oct 26, 2024

@phil-scott-78 Agreed, after looking over the thread I see exactly what you are mentioning. Very happy to help! Would we be ok with just the console project AOT compatible while Spectre CLI support coming later?

I think the main focus right now should be the Console support just so we can make TUI Apps for consoles in a minimal way with AOT, the cli would be handy too but for now users can just make their own parser for command line arguments so Cli doesnt necessarily have to work on aot yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 🚀
Development

Successfully merging a pull request may close this issue.

10 participants