BenchmarkDotNet icon indicating copy to clipboard operation
BenchmarkDotNet copied to clipboard

fix isnativeaot definition

Open kasperk81 opened this issue 1 year ago • 6 comments
trafficstars

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?

kasperk81 avatar Apr 14 '24 16:04 kasperk81

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.

jkotas avatar Apr 14 '24 16:04 jkotas

Maybe just add the IsAot check?

timcassell avatar Apr 15 '24 09:04 timcassell

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.

MichalStrehovsky avatar Apr 23 '24 07:04 MichalStrehovsky

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.

timcassell avatar Apr 23 '24 08:04 timcassell

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

kasperk81 avatar Apr 23 '24 19:04 kasperk81

Assembly.Location is true for PublishSingleFile as well

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

timcassell avatar Apr 23 '24 22:04 timcassell