Do not share assemblies of task dependencies if not running on MSBuild.exe.#13285
Do not share assemblies of task dependencies if not running on MSBuild.exe.#13285teo-tsirpanis wants to merge 3 commits intodotnet:mainfrom
MSBuild.exe.#13285Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where MSBuild, when loaded via MSBuildLocator into a host application, could fail if the host app had dependencies on older assembly versions than those required by MSBuild tasks. The fix changes how assemblies found in the MSBuild tools directory are loaded into AssemblyLoadContexts (ALCs).
Changes:
- Modified
MSBuildLoadContext.Loadto conditionally load dependencies found in the tools directory into either the default ALC (when running as MSBuild.exe) or the plugin's isolated ALC (when running in a host app via MSBuildLocator) - Added detection logic to determine if MSBuild is running as MSBuild.exe by checking the entry assembly name
AR-May
left a comment
There was a problem hiding this comment.
Overall looks good to me. I ran experimental insertion with these changes, for a bit of extra test coverage. Let's see whether the tests would pass.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AR-May
left a comment
There was a problem hiding this comment.
While looking at the additional tests I ran, we realized that the code in this version is not quite correct. MSBuildLoadContext is available and used only in .NET Core version of the MSBuild. Because of that, we should not distinguish between msbuild.exe and an app using MSBuildLocator, since msbuild.exe does not go through this code path at all. Instead, we need to distinguish between dotnet build/run and an app that uses MSBuildLocator and load differently in those cases.
Today, RunningInMSBuildExe is false when running dotnet build, which means this code always loads into a custom ALC. This can potentially cause a performance regression.
I talked to @AR-May about this offline, and I'm not sure this is the right behavior. I think this PR may be exactly correct and our definition for We're going to dig into that angle a bit more. |
Convinced that this isn't fully right given today's RunningInMSBuildExe.
Fixes microsoft/MSBuildLocator#336
Fixes #13286
Context
When MSBuild resolves dependencies for a task, if it cannot find the dependent assembly in the task assembly's directory, it tries finding it in MSBuild's tools directory. In that case, the dependency is loaded to
AssemblyLoadContext.Defaultinstead of the task's own ALC, in order to share it across tasks and avoid loading it multiple times.However, loading it to the default ALC does not reliably work if MSBuild is running on an arbitrary host app (such as by being loaded by
MSBuildLocator), because it will cause failures if the host app has itself a dependency on an older assembly version than the task's dependency. This is what caused the linked issue.Changes Made
Updated
MSBuildLoadContext.Loadto load dependencies found in the tools directory, into itself, instead of inAssemblyLoadContext.Default. To avoid regressing performance in the majority of cases, the existing behavior is maintained if MSBuild is running as part ofMSBuild.exe(detected by checking the entry assembly's name, once in static initialization).Testing
Validated that the test failures in the linked issue no longer reproduce. I did this by using a custom copy of .NET SDK 8.0.418, with a patched
Microsoft.Build.dll.Notes