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

Make NCronJob native AOT ready #54

Open
linkdotnet opened this issue May 15, 2024 · 7 comments
Open

Make NCronJob native AOT ready #54

linkdotnet opened this issue May 15, 2024 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@linkdotnet
Copy link
Member

Currently, NCronJob is not native AOT ready. That might be a less ideal choice for folks who run this library in the cloud and try to minimize the footprint.

There is a nice article from Microsoft itself: https://devblogs.microsoft.com/dotnet/creating-aot-compatible-libraries/

Basically what we want is:

  1. Add the analyzers:
<PropertyGroup>
    <IsAotCompatible>true</IsAotCompatible>
</PropertyGroup>
  1. Fix all errors
  2. Create a new sample with native aot and publish the app and check if it is still doing its thing.
@linkdotnet linkdotnet added the enhancement New feature or request label May 15, 2024
@pablopioli
Copy link

In the past have added AOT support to a few apps so I checked what I could do to help.

There are 3 incompatible parts:

  1. The easy. In NCronJobOptionBuilder the code must pass the warning upstream to the user of the library with the attribute
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)]
    The compiler will warn the client code if something is unsupported by AOT.

  2. In JobExecutor there is a call to
    ActivatorUtilities.CreateInstance
    this line will never be AOT compatible. It will need a breaking change, requiring all needed types to be registered in the IOC container. But is simple to do.

  3. In JobExecutor there is a call to
    MakeGenericType
    this is more complicated and will requiere a redesign to as how the notifications are handled.

Those are my two cents, and seeing this issue I thought on discussing it and see how can I help.

@pablopioli
Copy link

Just adding, the updated documentation says to use IsTrimmable instead of IsAotCompatible in libraries

https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming?pivots=dotnet-8-0

@linkdotnet
Copy link
Member Author

@pablopioli Interesting - I can't find where IsTrimmable is a replacement for IsAotCompatible - more or less IsTrimmable is a precursor for AOT.

In JobExecutor there is a call to ActivatorUtilities.CreateInstance

Yeah - this kind of a fallback or convenient method if you forgot to register a job. We could also just "fail" in those moments.
@falvarez1 and I had some discussion around that topic.

In JobExecutor there is a call to MakeGenericType this is more complicated and will requiere a redesign to as how the notifications are handled.

Yes - could we "ignore" that somehow and tell the compiler that we never run into the issue that the type might be trimmed out at this point in time? If the user didn't register IJobNotificationHandler<> properly you can't get into the code.

@pablopioli
Copy link

@pablopioli Interesting - I can't find where IsTrimmable is a replacement for IsAotCompatible - more or less IsTrimmable is a precursor for AOT.

https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot

The important one for a library is the trim analyzer. The trim process is very agressive and if unsure it will default to remove the member. With properties there is not much of a problem, you use it or not. But for a library like this the constructors are very problematic. NCronJob could just not find any constructor at all.

The other analyzers are for single file deployment (that is a decision for the consuming app) and the AOT analyzer (if your library can support trimming the tooling will simply convert the bytecode to native code.)

In JobExecutor there is a call to ActivatorUtilities.CreateInstance

Yeah - this kind of a fallback or convenient method if you forgot to register a job. We could also just "fail" in those moments. @falvarez1 and I had some discussion around that topic.

Yes, it is more a design decision than a technical one.

In JobExecutor there is a call to MakeGenericType this is more complicated and will requiere a redesign to as how the notifications are handled.

Yes - could we "ignore" that somehow and tell the compiler that we never run into the issue that the type might be trimmed out at this point in time? If the user didn't register IJobNotificationHandler<> properly you can't get into the code.

No, you can't suppress trimming warning. You can ignore them if you are sure that it will not fail anyway. The compilation process will warn you but the program will run without problems. Unless you are treating warning as errors and then you are toasted.

@linkdotnet
Copy link
Member Author

We can live with the current state. Given that this was just an idea and not a real "need", we can keep it in the backlog.

That said I appreciated your input a lot @pablopioli! Let me know if you have other ideas!

@nulltoken
Copy link
Collaborator

@linkdotnet Do we want to pave the way for this in v4?

As identified in #106, we could

Maybe add a .UseNCronJob() extension method that would harvest the JobRegistry
and ensure every described type has been previously registered.
Otherwise, throw an InvalidOperationException ("The following types haven't
been registered in the dependency injection container: ...").

And then drop the call to ActivatorUtilities.CreateInstance in JobExecutor.

@linkdotnet
Copy link
Member Author

I have to check some issues and prs - I can vaguely remember why the code is there, but have to dig a bit deeper.
basicslly we wanted to make it easier for users in certain scenarios

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants