Skip to content

Fix IsNonBundledAssemblyLoadingSupported condition in PlatformDetection #73235

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.

Sign up for GitHub

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

Merged
steveisok merged 3 commits into from
Aug 3, 2022
+3 −1
Merged

Fix IsNonBundledAssemblyLoadingSupported condition in PlatformDetection #73235

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23b8e6a
Fix IsNonBundledAssemblyLoadingSupported condition in PlatformDetection
Aug 2, 2022
0335593
Revert prop name and add explanation
Aug 2, 2022
3b6940d
Merge remote-tracking branch 'upstream/main' into fix-condition
Aug 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ private static bool GetLinqExpressionsBuiltWithIsInterpretingOnly()
public static bool IsNotIntMaxValueArrayIndexSupported => s_largeArrayIsNotSupported.Value;

public static bool IsAssemblyLoadingSupported => !IsNativeAot;
public static bool IsNonBundledAssemblyLoadingSupported => !IsAssemblyLoadingSupported && !IsMonoAOT;
public static bool IsAssemblyLoadingFromFileSupported => IsAssemblyLoadingSupported && !IsMonoAOT;
Copy link
Member

@stephentoub stephentoub Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@steveisok steveisok Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@MichalStrehovsky MichalStrehovsky Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@steveisok steveisok Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
}

Copy link
Member

@MichalStrehovsky MichalStrehovsky Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@steveisok steveisok Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public static bool IsMethodBodySupported => !IsNativeAot;
public static bool IsDebuggerTypeProxyAttributeSupported => !IsNativeAot;
public static bool HasAssemblyFiles => !string.IsNullOrEmpty(typeof(PlatformDetection).Assembly.Location);
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Runtime.Extensions/tests/System/AppDomainTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public void ExecuteAssemblyByName()
}).Dispose();
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNonBundledAssemblyLoadingSupported))]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsAssemblyLoadingFromFileSupported))]
public void ExecuteAssembly()
{
CopyTestAssemblies();
Expand Down