Fix IsNonBundledAssemblyLoadingSupported condition in PlatformDetection

[steveisok]

Inadvertently put in a condition that only made tests with this check run in NativeAOT. The intent was to skip on both NativeAOT and Mono AOT.

[ghost]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[stephentoub]

I'm curious what's different between Native AOT and Mono's AOT that the former is able to support loading assemblies from files and the latter isn't?

[steveisok]

In full AOT for mono, assembly loading is allowed, but in the case I'm trying to skip, the exe assembly wasn't AOT'd and was not registered as a module on startup. For NativeAOT, my understanding is everything compiles into a single binary. That means you can't load anything external.

I kept going back and forth on a proper name for the property. I reverted back to what I originally had as I now think it's a little more accurate.

[MichalStrehovsky]

In NativeAOT, Assembly.Load works too because that one is not actually loading anything new. The assembly was part of the app.

Assembly.LoadFrom or loading from byte array don't work and are blocked on the existing condition. Does it match your semantic?

[steveisok]

Close. I say that because tests like this pass:

runtime/src/libraries/System.Runtime/tests/System/ActivatorTests.cs

Lines 626 to 648 in 46db4aa

	public static void TestingCreateInstanceFromObjectHandle(string assemblyFile, string type, string returnedFullNameType, Type exceptionType)
	{
	ObjectHandle oh = null;
	if (exceptionType != null)
	{
	Assert.Throws(exceptionType, () => Activator.CreateInstanceFrom(assemblyFile: assemblyFile, typeName: type));
	}
	else
	{
	oh = Activator.CreateInstanceFrom(assemblyFile: assemblyFile, typeName: type);
	CheckValidity(oh, returnedFullNameType);
	}
	if (exceptionType != null)
	{
	Assert.Throws(exceptionType, () => Activator.CreateInstanceFrom(assemblyFile: assemblyFile, typeName: type, null));
	}
	else
	{
	oh = Activator.CreateInstanceFrom(assemblyFile: assemblyFile, typeName: type, null);
	CheckValidity(oh, returnedFullNameType);
	}
	}

[MichalStrehovsky]

I'll leave it up to you, but I would expect that API to be unsupported in general. It might work in corner cases like in the test but it's a much easier story to say to customers that apis that load assemblies from files are not supported than to have an asterisk with when it is supported. Especially if there's perfectly good replacement apis like CreateInstance(string,string).

Apis that are unsupported don't need test coverage.

[steveisok]

Tagging @lambdageek in the event I didn't explain it quite right.

[mdh1418]

The thing that isn't supported, is it specifically loading an assembly from a file on mono, or is it the assembly execution from a file on mono that is not supported? Isn't this AssemblyLoading from a file as well

runtime/src/libraries/System.Reflection/tests/AssemblyTests.cs

Line 348 in 1e7299d

public void LoadFile()

?

[MichalStrehovsky]

The thing that isn't supported, is it specifically loading an assembly from a file on mono, or is it the assembly execution from a file on mono that is not supported? Isn't this AssemblyLoading from a file as well

This one is in the same category as the Activator.CreateInstanceFrom case above - it's loading a file, but the file happens to be part of the app, so it probably works.

The problem (and test hole) is that the tests are not testing the primary customer scenario for various LoadFrom APIs - the APIs are meant for e.g. a plugin that wasn't part of the app.

If we were to adapt the tests to test the primary customer scenarios and not the corner case that happens to work (the loaded assembly was part of the app already), annotating these tests as PlatformDetection.IsAssemblyLoadingSupported and saying that loading assemblies from files is not supported on Mono AOT (even though it might work, but it's not tested) would be a good fix. I'm not sure introducing new PlatformDetection just to keep test coverage for what is actually a non-mainstream scenario has a lot of value.

But don't consider this feedback blocking. What is here works too.