Microsoft.Build.Shared.MSBuildLoadContext misbehavior

[ericstj]

Issue Description

When investigating https://github.com/dotnet/sdk/pull/49531 we found some pretty bad behavior of MSBuildLoadContext.

1. It doesn't honor framework unification. If a assembly has in its directory a framework assembly (like System.Memory, or System.Text.Json) it will allow that version to be loaded, even if it's older than the shared framework.
2. It doesn't do roll-forward. If an assembly reference is older than the assembly provided (eg: reference is for 4.0.1.2, and dll shipped is 4.0.2.0) it skips loading it and falls back to the default ALC.

Steps to Reproduce

We reproduced this by SDK testing that specified MSBUILDADDITIONALSDKRESOLVERSFOLDER = C:\h\w\B80C09CA\p\r\Release\net472\SdkResolvers so it was telling MSBuild to load .NET 4.7.2 SDK resolvers for a .NET Core invocation of msbuild. This is a bit of a stretch, but I bet one could construct a similar repro without this accidental mismatch.

Suppose a task has a mix of dependency references and relies on framework unification. That works so long as framework unification produces the same result as task-directory unification. Now imagine that MSBuild is running on a newer framework - it would not longer produce the same result and the task would fail to load.

Expected Behavior

Assembly loads using it's local dependencies only when they are newer than the framework assemblies. Task unifies to either framework assembly or local assembly consistently.

Actual Behavior

Task will use local dependency when older than framework dependency. Task does not unify to local dependency when chosen, will fall back to framework dependency.

Analysis

Version comparison bug here https://github.com/dotnet/msbuild/blob/b85b8a5813da73efd3920c98a03d1c7666c83006/src/Shared/MSBuildLoadContext.cs#L86-L89

I'm not sure what the right way is to to probe framework dependencies for a version comparison - today the only thing the ALC does is return null for MSBuild (which doesn't permit a version comparison). It's possible this is not easy/posible without a deps file. I bet @elinor-fung or @agocke could tell us.

Versions & Configurations

No response

[teo-tsirpanis]

@ericstj, would https://github.com/dotnet/msbuild/commit/24f88977c866c4794436fc76e7a9a1a4a7d034e1 fix this?

[ericstj]

The case we looked at did not have a .deps.json so I don't think that fixes it. We were looking at task loading on core with no deps.json. That task was allowed to load a lower System.Text.Json than present in the framework (case 1), and was also allowed to load a lower System.Text.Json than was required by other assemblies that it loaded (case 2).

[teo-tsirpanis]

Can't the task add a .deps.json then? 😅

There is a well-defined dependency loading behavior if a task has a .deps.json file — and my proposal will make it even more well-defined, and changing the legacy behavior has compatibility risks; the version comparison had been loosened in #7042, but was reverted a few months later in #7415.

If you are referring to https://github.com/dotnet/sdk/pull/49531#issuecomment-3198636682, loading .NET Framework assemblies to .NET is already into semi-supported territory. I don't know how easy it is to change the SDK tests to use the .NET Core resolvers, but maybe you can try adding a <GenerateDependencyFile>true</GenerateDependencyFile> to generate a .deps.json even on .NET Framework, to see if it will fix the issue.