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

fix isnativeaot definition #2561

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

kasperk81
Copy link

current detection fails to differentiate between PublishSingleFile and PublishAot because they both return null from Assembly.Location.

@adamsitnik @jkotas is there any existing app model API available to make this detection less fragile?

@jkotas
Copy link
Member

jkotas commented Apr 14, 2024

There is no public API to detect whether the app has been published with PublishAot=true.

NativeAOT is a collection of individual behaviors (single-file, no dynamic code, AOT pre-compilation, trimming, IL trimmed from the final binary, ...). We recommend that libraries check for these individual behaviors.

@timcassell
Copy link
Collaborator

Maybe just add the IsAot check?

@MichalStrehovsky
Copy link
Member

Maybe just add the IsAot check?

There are no plans to add one. We have capability checks: for example, when accessing Assembly.Location, one can check if it's empty and that handles both single file or native AOT app or whatever future single file model. Similarly, RuntimeFeature.IsDynamicCodeSupported can be checked to ensure Reflection.Emit works and that covers native AOT or Mono AOT with interpreter disabled or whatever other model where Emit doesn't work.

If one is just curious if PublishAot was in the project for informational purposes like it seems here (not for if checks; if checks should use the appropriate capability check for the specific capability), I suggest adding:

<ItemGroup>
  <AssemblyMetadata Include="BuildProperties.PublishAot" Value="$(PublishAot)" />
</ItemGroup>

to the project file, and using:

foreach (var a in Assembly.GetEntryAssembly().GetCustomAttributes<AssemblyMetadataAttribute>())
    if (a.Key == "BuildProperties.PublishAot")
        Console.WriteLine($"PublishAot: '{a.Value}'");

to check for it. I'd remove the IsNativeAot helper and inline this wherever needed. IsNativeAot helper will just distract people from using the right capability checks. I've seen a handful of projects inventing their own "IsNativeAot" check that then introduces bugs for PublishSingleFile and similar because people will use it when they should have used a capability check.

@timcassell
Copy link
Collaborator

timcassell commented Apr 23, 2024

Maybe just add the IsAot check?

There are no plans to add one.

I meant the RuntimeInformation.IsAot property (in the same file) which just wraps RuntimeFeature.IsDynamicCodeSupported. The tests are failing the way it's used in this PR because that API doesn't exist in netstandard2.0. And I don't think the Assembly.Location check needs to be removed.

If Assembly.Location and RuntimeFeature.IsDynamicCodeSupported return the same values for both MonoAot and NativeAot, we can add an additional check for IsNewMono (like we did with IsWasm).

I don't think we can do that AssemblyMetadata trick as a library.


Anyways, @kasperk81 hasn't even described the problem that this code produced. RuntimeInformation class is internal, and IsNativeAot is used in very few places.

@kasperk81
Copy link
Author

If Assembly.Location and RuntimeFeature.IsDynamicCodeSupported return the same values for both MonoAot and NativeAot, we can add an additional check for IsNewMono (like we did with IsWasm).

Assembly.Location is true for PublishSingleFile as well

@timcassell
Copy link
Collaborator

Assembly.Location is true for PublishSingleFile as well

Right, and it looks like we incorrectly check that for IsNetCore also.

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

Successfully merging this pull request may close these issues.

None yet

4 participants