Use AssemblyLoadContext Name instead of AppDomain

[bording]

Fixes #9348

Context

When MSBuild was updated to isolate tasks into a custom AssemblyLoadContext, AssemblyLoadsTracker was not updated to be aware of this change. This means that the log messages always say AppDomain: [Default] and don't show any information about which AssemblyLoadContext was used to load the assembly.

Changes Made

When FEATURE_ASSEMBLYLOADCONTEXT is defined:

• AssemblyLoadsTracker now populates AssemblyLoadBuildEventArgs with the name of the AssemblyLoadContext used to load the assembly . The appDomainDescriptor parameter is re-used to do this.
• A new string resource has been added to update the text of AssemblyLoadBuildEventArgs.Message to use label the AppDomainDescriptor value as "AssemblyLoadContext" instead of "AppDomain".

Testing

I couldn't find any existing tests that covered AssemblyLoadsTracker behavior, so I haven't updated or added any tests yet. Unit tests added.

Notes

AssemblyLoadContext.GetLoadContext can return null, but only if the assembly loaded isn't derived from RuntimeAssembly. The only way I'm aware that would be true is if MetadataLoadContext was used to load the assembly as ReflectionOnly, and I wouldn't think that would be a relevant case here. For now, if it was null for some reason, I've made appDomainDescriptor have the value "unknown". Otherwise, if I let the null through, AssemblyLoadBuildEventArgs would print "[Default]" in its message, which would not be correct.

I added a new string resource in a separate commit because I wasn't sure if that would be okay or not. I know that means there's some translation work to do. Looking at the existing translations for TaskAssemblyLoaded, since "AppDomain" isn't translated, it looks like it should just be a matter of copying the existing one and replacing that one word.

[JanKrivanek]

Testing

I couldn't find any existing tests that covered AssemblyLoadsTracker behavior, so I haven't updated or added any tests yet.

Could you possibly reuse/repurpose this integration test for assembly loads tracking?: https://github.com/dotnet/msbuild/blob/main/src/Build.UnitTests/BinaryLogger_Tests.cs#L192-L250 (plus if needed we have DotNetOnlyFact\Theory and WindowsFullFrameworkOnlyFact\Theory attributes to annotate tests running only on particular platform)

I added a new string resource in a separate commit because I wasn't sure if that would be okay or not. I know that means there's some translation work to do. Looking at the existing translations for TaskAssemblyLoaded, since "AppDomain" isn't translated, it looks like it should just be a matter of copying the existing one and replacing that one word.

All good.
Translations happens via separate automations in delayed automated PRs and it should be able to reuse (though in this case it probably won't as the strings differ - but that's no harm at all).

[JanKrivanek]

Thank you for this contribution!

It looks good to me as is.

Please try if you can have a simple test verifying the NET behavior.
Once this is cleared (or once we know reasons why it's complicated - so I can jump in to help) I'm happy to sign off

[bording]

Could you possibly reuse/repurpose this integration test for assembly loads tracking?: https://github.com/dotnet/msbuild/blob/main/src/Build.UnitTests/BinaryLogger_Tests.cs#L192-L250 (plus if needed we have DotNetOnlyFact\Theory and WindowsFullFrameworkOnlyFact\Theory attributes to annotate tests running only on particular platform)

I did see that test, so I'll take a look at extending it out a bit to cover the relevant part of the log change.

[bording]

@JanKrivanek I took the test and split it into two versions, one that looks for the presence of AppDomain: [Default] being in the text, and the other looking for AssemblyLoadContext: Default.

Do you think it would be worth having a test that actually shows a task loading into a non-default AssemblyLoadContext? If so, can you point me to an example test of how to actually get a task assembly loaded in a test?

[JanKrivanek]

@JanKrivanek I took the test and split it into two versions, one that looks for the presence of AppDomain: [Default] being in the text, and the other looking for AssemblyLoadContext: Default.

Do you think it would be worth having a test that actually shows a task loading into a non-default AssemblyLoadContext? If so, can you point me to an example test of how to actually get a task assembly loaded in a test?

I leave up on you :-)

If you'd have to test another load context you'd need to have a task in separate assembly - all our tests use tasks that are compiled within the already loaded test assemblies, so you'd either need to add a separate project (too heavyweight for the test) or build one-off temp assembly within the test.
Then you can have test test the custom load context - similar to:

Assembly loaded during TaskRun: another-lib, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null (location: <path>\another-lib.dll, MVID: 19ab2034-81ba-45df-b164-1ba0cb66da6b, AssemblyLoadContext: MSBuild plugin <path>\another-lib.dll)
Using "SimpleTask1" task from assembly "<path>\another-lib.dll".

If you'll leave the test as is (totally fine for the purpose they're validating) - those 2 differ only in that single assertion string - it'd be more readable if those 2 call a single helper giving it the assertion string as param.

[WindowsFullFrameworkOnlyFact]
Test1()
{
  TestImpl("Assertion text a");
}

[DotNetOnlyFact]
Test2()
{
  TestImpl("Assertion text b");
}

[rainersigwald]

all our tests use tasks that are compiled within the already loaded test assemblies

Not all of them! One is actually perfect for this; check out https://github.com/dotnet/msbuild/blob/08494c73128451a3f7cfb47a5e9cbd63f5507a1f/src/Shared/UnitTests/TypeLoader_Dependencies_Tests.cs

[bording]

@JanKrivanek I pushed up a new commit that adjusts the first test in the way you suggested, and I augmented the test @rainersigwald pointed out as well. How does that look?

[rainersigwald]

Awesome, thanks!

[JanKrivanek]

Thank you!