Each MSBuild task should exist in its own AssemblyLoadContext

[AArnott]

Each MSBuild Task should load in its own AssemblyLoadContext. At least 3rd party tasks. This prevents build failures due to two independently developed tasks sharing a dependency but with different versions.

Also, the default AssemblyLoadContext should be very nearly empty, so that each Task can define its own dependencies instead of inheriting them from the default context. For example, Roslyn assemblies should not be in the default AssemblyLoadContext so that MSBuild tasks can use Roslyn assemblies of the version it was compiled against.

@tmat summarized this well after a discussion we had in another issue.

CC: @nguerrera

[jeffschwMSFT]

cc @swaroop-sridhar

[ericstj]

@joperezr hit this with a task that was trying to use Roslyn and had to workaround it by turning it into an out of proc tool.

[rainersigwald]

See design proposal #4133 by @vitek-karas. Feedback welcome!

[ericstj]

FYI we just hit this again here: dotnet/corefx#36103

In that case the task was trying to use a newer NuGet than MSBuild had in its deps file. /cc @safern @tmat

[SimonCropp]

@AArnott i assume this shouldnt be a problem for strong named task assemblies? since they should be able to be loaded side-by-side?

[AArnott]

@SimonCropp It is indeed a problem for strong-named assemblies. In fact all my task assemblies are always strong-named. The problem isn't just the task assembly itself, it's its whole dependency tree. And while all its dependency may be strong-name signed, the dependencies should still match exactly what the task is expecting. Some dependencies have a common assembly version across several actual versions, such that the first one would load in the CLR and the rest of the tasks that need that same dependency (but a different version) would reuse the same version as the first task asked for. That's a problem.

[jzabroski]

Your ultimate test case is "turtles all the way down":

Create a TaskRunner.exe which loads an interface ITask { Execute(string[] args); } based on an interface ITaskDefinition<T> where T : new, ITask { Guid Key { get; set; } }

This would allow you to correctly handle the scenario of:

1. Installing a .NET Core CLI Tool as a local tool from an MSBuild Pre-Build step
2. Running that .NET Core CLI Tool as a local tool from an MSBuild Post-Build step
   a. With support for two scenarios:
   1. Being able to reference an older version of a system dependency, such as System.ComponentModel.Annotations. Consider my scenario, where a concrete ITask instance references via its implementation details the ability to provide user-friendly names for enumeration values. I LOVE THIS EXAMPLE because I firmly believe Microsoft has wildly over-complicated this, and this NAILS the complexity issue.
      🎉 🎂 🎁

This is exactly the problem we run into every day. I get more support requests in FluentMigrator for this issue than all other issues combined.

I'll try to push a sample project which demonstrates this issue.

[SimonCropp]

in fody this is handled by spinning up a custom appdomain in .net and a custom load context in netcore. this domain/context is then cached per solution so that subsequent builds dont need to spin up a new domain/context. this avoids the performance problem with AppDomainIsolatedTask, which forces a new appdomain for every run of the task, even if all the assemblies are the same. that and AppDomainIsolatedTask is not supported in netcore

i suspect the majority of msbuild tasks (that have dependencies) have the same "single appdomain" bug and they have never noticed.

i would very much like to remove this complexity from Fody.

[jzabroski]

@SimonCropp Can you point me to the code you use in Fody? I searched https://github.com/Fody/Fody/search?q=AppDomain&unscoped_q=AppDomain and I see:

1. https://github.com/Fody/Fody/blob/9236ca84a1f660b067545bf3df5882fd0c0b7a9c/FodyCommon/IsolatedAssemblyLoadContext.cs
2. https://github.com/Fody/Fody/blob/23b9e722dbd79990f3012c841f98d135f1c34e41/Fody/DomainAssemblyResolver.cs
3. https://github.com/Fody/Fody/blob/9236ca84a1f660b067545bf3df5882fd0c0b7a9c/FodyIsolated/InnerWeaver.cs

I believe, based on your comments, you are only referring to IsolatedAssemblyLoadContext, but not to the resolution handlers you write in InnerWeaver and DomainAssemblyResolver. I don't think putting each MSBuild task in its own AssemblyLoadContext will necessarily solve all your problems, because I think you'll end up with "private diamond dependencies" where people cannot independently separate interfaces from implementation.

BenchmarkDotNet does similar nastiness, here, in the following sequence:

1. BenchmarkRunnerDirty.RunWithDirtyAssemblyResolveHelper
2. DirtyAssemblyResolveHelper.HelpTheFrameworkToResolveTheAssembly
3. BenchmarkSwitcher.Run
   • Contains this lovely comment:

     // VS generates bad assembly binding redirects for ValueTuple for Full .NET Framework
     // we need to keep the logic that uses it in a separate method and create DirtyAssemblyResolveHelper first
     // so it can ignore the version mismatch ;)

[SimonCropp]

@jzabroski fory has two contexts

the root task

https://github.com/Fody/Fody/tree/master/Fody

contains no 3rd party dependencies and is loaded into the shared msbuild appdomain

loads "isolated" here https://github.com/Fody/Fody/blob/master/Fody/Processor.cs#L128

contexts are cached per sln in a dictionary https://github.com/Fody/Fody/blob/master/Fody/Processor.cs#L27

an isolated part

https://github.com/Fody/Fody/tree/master/FodyIsolated

can ref and load any 3rd part assemblies

note that fody is also a plugin based model. so the above give isolation for any weavers as well https://github.com/Fody/Home/blob/master/pages/addins.md

[neuecc]

I've encountered (maybe) related issues.

when using MessagePack.MSBuild.Tasks and MagicOnion.MSBuild.Tasks which are custom task both using Microsoft.CodeAnalysis.CSharp

  <Target Name="GenerateMessagePack" AfterTargets="Compile">
    <MessagePackGenerator Input=".\ChatApp.Shared.csproj" Output="..\ChatApp.Unity\Assets\Scripts\Generated\MessagePack.Generated.cs" />
  </Target>
  <Target Name="GenerateMagicOnion" AfterTargets="Compile">
    <MagicOnionGenerator Input=".\ChatApp.Shared.csproj" Output="..\ChatApp.Unity\Assets\Scripts\Generated\MagicOnion.Generated.cs" />
  </Target>

dotnet build shows following error.

Could not load file or assembly 'Microsoft.CodeAnalysis.CSharp, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Could not find or load a specific file. 

because MessagePack.MSBuild.Tasks depends Microsoft.CodeAnalysis.CSharp 3.1.0 but MagicOnion.MSBuild.Tasks depends 3.4.0.

for workaround, I've adjusted to use same version, it works.
But this versioning hell is painful.

[rainersigwald]

@neuecc Yeah, that problem should be resolved by this change. If you put a sample project up somewhere, I can confirm by testing #4916 against it before merging.

[neuecc]

@rainersigwald thanks, I've pushed sample repo here https://github.com/neuecc/MSBuildAssemblyLoadIssue

[rainersigwald]

@neuecc Thanks!

$ dotnet build
Microsoft (R) Build Engine version 16.5.0-dev-20058-01+6d4c3b1e2 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restore completed in 14.77 sec for S:\play\MSBuildAssemblyLoadIssue\MSBuildAssemblyLoadIssue.csproj.
  You are using a preview version of .NET Core. See: https://aka.ms/dotnet-core-preview
[Out]S:\play\MSBuildAssemblyLoadIssue\MagicOnion.Generated.cs
  MSBuildAssemblyLoadIssue -> S:\play\MSBuildAssemblyLoadIssue\bin\Debug\netstandard2.0\MSBuildAssemblyLoadIssue.dll

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:26.59

(That's with MSBuild from my PR)

[rainersigwald]

@jzabroski please start a new issue for that.