dotnet / BenchmarkDotNet

Powerful .NET library for benchmarking
https://benchmarkdotnet.org
MIT License
10.24k stars 952 forks source link

fix isnativeaot definition #2561

Open kasperk81 opened 3 months ago

kasperk81 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Maybe just add the IsAot check?

MichalStrehovsky commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Assembly.Location is true for PublishSingleFile as well

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