Implement binding redirect support for msbuild tasks

[tmat]

Currently tasks that require binding redirects for their dependencies need to hook AssemblyResolve event and implement the redirection manually. Implementing redirection correctly is non-trivial and error prone, especially when the task is built as a portable library and needs to redirect CoreFX assembly versions.

Since CoreFX 1.1 heavily relies on binding redirection, many CoreFX libraries and facades now require redirection on Desktop, any portable task that uses CoreFX 1.1 packages will need to redirect many CoreFX assemblies.

I propose msbuild adds a global AssemblyResolve hook that implements semantic versioning logic (lower version gets redirected to the higher version), or at least logic that loads and applies redirects from .dll.config of the task being loaded (if exists).

Examples of existing tasks that redirect versions:

• buildtools:
  https://github.com/dotnet/buildtools/blob/master/src/common/AssemblyResolver.cs

[tmat]

//cc @ericstj

[KirillOsenkov]

A downside of the AssemblyResolve event is that it can only be installed in the primary appdomain. Tasks that run in a separate appdomain (such as Markup Compiler) can't take advantage of it. Mentioning it just in case, I'm not trying to make any particular point.

[tmat]

AssemblyResolve is just one option. Another, perhaps better, is that msbuild creates a separate domain for each custom task and configures it with its app.config file.

[natemcmaster]

cc @dougbu this looks like the issue you and I were just discussing

[dougbu]

@natemcmaster yup.

@srivatsn the ASP.NET team builds task assemblies that use System.Reflection.Metadata. We use these tasks in Visual studio and dotnet msbuild. We basically have very few choices

1. Create task packages that can't be used because they either don't contain System.Collections.Immutable.dll at all or contain System.Collections.Immutable.dll from their v1.3.0 package (assembly v1.2.1) -- the package the System.Reflection.Metadata package depends on.
   • VS 2017 installs this System.Collections.Immutable assembly in the same directory as MSBuild.exe. But, MSBuild.exe.config doesn't contain binding redirects mentioning it.
2. Create task packages that contain System.Collections.Immutable v1.2.0. Unfortunately this results in build warnings because the task projects need to depend on an older System.Collections.Immutable package than System.Reflection.Metadata requires. We're trying to reduce build warnings.
3. Beg the team that owns the System.Reflection.Metadata package to rebuild the contained assembly and cross our fingers we won't depend on inconsistent packages in the future. A stop-gap at best, partially because System.Collections.Immutable is often in the GAC.
4. Beg your team to fix this bug, allowing us to do 1. without new build warnings. So, I'm begging 😺
   • Updating the MSBuild.exe.config file would also work.

One solution that might work here would be to take the VSTest approach. That host uses binding redirects found in {TestAssembly}.dll.config. Not sure if that's specific to .NET Framework runs. But, a desktop-specific fix might be fine for our MSBuild case.

[srivatsn]

@AndyGerlicher is the right person to beg to :)

[ericstj]

@AndyGerlicher I think we need to do this. Here's a good example where the lack of this support + MSBuild adding assemblies to its own directory breaks a task that's trying to unify dependencies: dotnet/buildtools#2177.

There will be tasks in the next release of VS that need to use assemblies like System.Reflection.Metadata and System.Reflection.TypeLoader. These will end up having unification issues over time.

I think the suggestions of having tasks opt-in to app-domain isolation per-dll by providing a .dll.config is a reasonable approach.

[AArnott]

We already have a way for an MSBuild task to run in an isolated AppDomain: derive from AppDomainIsolatedTask. As to whether this also causes MSBuild to apply the task.dll.config file to that AppDomain, I don't know.

[AArnott]

A downside of the AssemblyResolve event is that it can only be installed in the primary appdomain.

What do you mean, @KirillOsenkov. AFAIK such an event handler can be added to any appdomain.

[KirillOsenkov]

@AArnott yes, if you control the appdomain. However I've since found a way to intercept AppDomain creation and install the handler in any appdomain, here's an example: KirillOsenkov/MSBuildStructuredLog@27e079f

[ericstj]

IMHO the current AppDomainIsolatedTask / LoadInSeparateAppDomainAttribute is too granular as it creates a domain per task instead of per assembly. It doesn't define a config file convention, instead it tries to apply MSBuild's config file: https://github.com/Microsoft/msbuild/blob/e70a3159d64f9ed6ec3b60253ef863fa883a99b1/src/Shared/TaskLoader.cs#L85-L115.

[jzabroski]

@ericstj Can you explain what you mean in the quote below? What does it mean for a "task [to try to] unify [its] dependencies"? I think this is an "epic" rather than a single GitHub issue. Allow me to explain what I think it means for a task to try to unify its dependencies.

@AndyGerlicher I think we need to do this. Here's a good example where the lack of this support + MSBuild adding assemblies to its own directory breaks a task that's trying to unify dependencies: dotnet/buildtools#2177.

The reason I ask is, I think part of the problem is it depends also on whether you run 32-bit or 64-bit MSBuild, if your Task is loading native DLLs.

Separately, it can get crazier for non-native DLL loading scenarios:

In my case, I was running a Task (FluentMigrator.MSBuild), which in turn executed code in a DLL it loaded (containing database migrations), which in turn loaded CsvHelper, which in turn has a dependency on "System.ValueTuple >= 4.4" (which is incorrect, because my build server is on .NET Framework 4.6.2 and that means it needs exactly System.Value Type = 4.4). I cannot force a bindingredirect here, and so while the code accidentally worked on my local machine (which defaulted to using .NET Framework 4.7.2 b/c I have Visual Studio 15.9.9) it doesn't work on my build server.

I think there are a couple of things tooling-wise that need addressing:

• To the best of my knowledge, the new .NET SDK project format <PackageReference> syntax (omitting transitive dependencies) wouldn't save me, because:
  1. Visual Studio's Manage NuGet Packages for Solution dialog suggests its OK to update to the latest System.ValueTuple, despite the fact its not OK for all the environments my code might be deployed against. I think the reason is that there is no mitigating control for when authors incorrectly specify dependencies in their nuspec.
  2. There is no clear understanding of what <PackageReference Include="System.ValueTuple"> means vs. the stack trace talking about version 4.0.2.0 (example of unhelpful stack trace below), because System.ValueType 4.4 is actually 4.0.2 and System.ValueType 4.5 is actually 4.0.3:

  [Migrate] D:\Teamcity\buildAgent\work\a26f47fb9f25ddb0\build.targets(313, 5): While executing migrations the following error was encountered: Could not load file or assembly 'System.ValueTuple, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified.,    at CsvHelper.ReadingContext..ctor(TextReader reader, Configuration configuration, Boolean leaveOpen)
  at CsvHelper.CsvReader..ctor(TextReader reader)
  at JohnZabroski.Database.EmbeddedResourceUtility.GetCsvData[T](Assembly assembly, String fileNameWithExtension)
  

  I believe project.assets.json was probably a step towards unifying "package versions" to ".net assembly versions". - If you see how Microsoft is using project.assets.json today with dotnet-retire to analyze project assets for software vulnerabilities/out-of-date dependencies. - In some sense, the only problem with project.assets.json is it is compile-time, and not run-time, and doesn't "certify" (via witness) that an assembly was loaded. After all, CsvHelper can say whatever it wants (depends on System.ValueTuple >= 4.4), but if nobody in the world can get this to run using System.ValueTuple 4.5 on .NET Framework 4.6.2, who cares what it says it thinks it can do? I want a witness that says, "Yup, this is what works for me." And THAT witness concept is what people use bindingredirects for today, more than ANYTHING ELSE. If you look on StackOverflow for the phrase "bindingredirect" and have an intern at Microsoft analyze the scenarios, I will bet you a case of Heady Topper beer that the NUMBER ONE cause is "sharing testimony" ("I witness this bindingredirect works").
• Redirecting assembly versions via bindingredirect is antiquated, and not actually what we want to accomplish. It is one level removed from how developers in the last 10 years since NuGet.exe have learned to start thinking about their dependencies. I realize this is highly controversial, and I'm not asking anyone to solve this problem right now, but I just want to explain why I think this is fundamentally broken no matter what approach we come up with. I have tweeted many times about this issue, but never fully explained it in a coherent way.
• Working around general sloppiness in .NET Standard and .NET Core due to speed of development causing human error. See Microsoft apologies here: Issues with .NET Standard 2.0 with .NET Framework & NuGet standard#481 - These are understandable. The most egregious is System.Configuration.ConfigurationManager references causing FileLoadException: Can't use System.Configuration.Configuration manager in a .NET Standard2.0 library on .NET FX4.6 standard#506

[ericstj]

a task to try to unify its dependencies.

We should think of task dlls like applications that can define a closure of dependencies native and managed. They should be the ones in control of defining that closure. The only things that MSBuild needs to control is the types that exchange across the plugin boundary (EG: ITaskItem, ITask, etc). MSBuild should do everything it can to make such a plugin model easy with a minimal requirement on the task provider.

@jzabroski +1 to all those scenarios. Trust me when I say I really feel this pain. I think MSBuild is uniquely positioned here to create a nice plugin experience since it can control the production and consumption side and can force tasks to be full descriptive about their dependencies on the production side (build of task DLL).

/cc @rainersigwald @livarcocc

[jzabroski]

@ericstj Just wondering if you (or anyone you work with) have any experience outside of .NET, like OSGI standards body for how Java loads plugins. It's been 10+ years since I worked in Java, but I recall Sun Microsystems and open source community working on similar problems years ago. In particular, the dude behind the NetBeans IDE was a big stickler for managing dependencies well, due to the fact that in the general case, solving dependencies is a 3SAT problem. See: http://lambda-the-ultimate.org/node/3688#comment-52458 as a potential starting point. You could also just reach out to the founder and initial architect of NetBeans, Jaroslav Tuloch. I know Microsoft hired Erich Gamma for Visual Studio Code, so maybe he can put you in touch with Jaroslav. Maybe they've talked over the years while Erich built Eclipse.

I think MSBuild is uniquely positioned here to create a nice plugin experience since it can control the production and consumption side and can force tasks to be full descriptive about their dependencies on the production side (build of task DLL).

This is an interesting assertion. Is it true, both in theory and practice? In general, years ago, there was this tool called cfEngine which argued the only way around system configuration issues was to compose systems out of atomic, indivisible items called atoms, and system updates are done through inductive reasoning about states of atoms and the system converges to a safe, well-defined target state via state transformations on atoms. This is in contrast to current tools like Chef/Puppet/Salt/etc which configure systems using procedural embedding of knowledge, where each "recipe" is an Actor that can mutate a global data store. CFEngine was arguably "right in theory" but "wrong in practice" (as pop culture has shown with the enormous success of Chef and Puppet).

As for the sub-assertion about MSBuild being uniquely well-suited, I'm not so sure that matters. The only thing that matters is the ability, in capability-security terms, is the ability to define a "Powerbox". That is what MSBuild effectively does, by having Tasks register their dependencies: The key requirement is NO ambient dependencies. Looked at this way, long-term, the solution is not to build an empire within MSBuild but perhaps to extend and improve an initiative like Nate McMaster's https://github.com/natemcmaster/DotNetCorePlugins - In this sense, you are right when you say:

We should think of task dlls like applications that can define a closure of dependencies native and managed.

But... I would re-phrase: Task DLLs are applications that can define closures with no free variables. This principle was actually written in an OOPSLA paper many years ago by Andy Black when he was at OGI doing programming language research. His key question is, what does it mean for an object to be a component? His definition was a closure with no free variables. Boom.

[ericstj]

Good points. There are folks who think about plugins here at Microsoft, I don't claim to be one of them, my main stake in this game is a customer of this feature and an opinion holder on what sort of characteristics it should have. @livarcocc do you think you can triage this issue to let us know when MSBuild is likely to consider it?

[jzabroski]

@ericstj I had an epiphany. I literally no longer need MSBuild. For the one use case where it might be useful, building a directed acyclic graph of project dependencies and piping each project into a work-stealing deque for concurrent project builds, I can delegate to MSBuild. But for every other scenario, I literally no longer need MSBuild as a c# developer because I have dotnet.exe and dotnet.exe Tools.

I suggest Microsoft not spend the effort to fix bindingRedirect for msbuild, since the problem literally comes from the lame behavior prior to .NET Core.

[AArnott]

@jzabroski at least with msbuild (desktop) it's a solvable problem. Once msbuild core is mixed in (which is what dotnet build is, BTW) it's impossible to solve at present.

[jzabroski]

@AArnott Sorry, can you elaborate on how it's solvable on .NET Framwork 4.5.2? I believe the problem may be harder than I realize based on your comments.

In my case, I can run dotnet.exe tool dotnet-fm fine on .NET Core, whereas MsBuild target FluentMigrator.MSBuild fails.

[SPOILER: I am thinking your point is that if I'm trying to provide tooling for users targeting .NET Framework 4.5.2 and lower AND .NET Core apps, then I'm screwed because now I have two different code bases to maintain and my build process becomes non-homogeneous, and if I have to fix an issue, I fix it in two places because there is no Target Framework Moniker for .NET 4.5.2? I'm just trying to understand exactly what problem you see with moving logic into DotNetCli tooling. Maybe you have a different point in mind.]

I guess it would help for you to explain why:

a) it's solvable problem for msbuild.exe (desktop)
b) it's impossible to solve for dotnet build (and therefore dotnet msbuild)

1. In this scenario, why can't I just hook AppDomain.AssemblyResolve event as a cheap out?

Apologies if you think this discussion is drivel. I tend to cast a wide net and pierce through topics until I have a deep, all-encompassing understanding of the problems. It can annoy some people who just want "bindingRedirect for msbuild tasks" done so they can get on with their immediate problem.

[AArnott]

trying to provide tooling for users targeting .NET Framework 4.5.2 and lower AND .NET Core apps, then I'm screwed because now I have two different code bases

No, it has nothing to do with what your users target, and everything to do with whether you want to support them in running msbuild.exe as well as dotnet.exe build (which is MSBuild Core). Both of these build engines can build projects that target either .NET Core or .NET Framework (or others).

now I have two different code bases to maintain and my build process becomes non-homogeneous, and if I have to fix an issue, I fix it in two places

Yes, you have to compile twice: once targeting .NET Framework and referencing MSBuild 14+, and again targeting .NET Core and referencing MSBuild Core. If you code it right, you can share code between the two built DLLs and even share the project for both using multi-targeting.

there is no Target Framework Moniker for .NET 4.5.2

Yes there is: net452

why it's solvable problem for msbuild.exe (desktop)

Because of AppDomains. I can set up my own AppDomain within an MSBuild task and do whatever I want, including set up binding redirects. This works, and I've even made a nuget package to help you because it's very tedious to do this right.

why it's impossible to solve for dotnet build (and therefore dotnet msbuild)

Because .NET Core doesn't have appdomains. It has AssemblyLoadContext instead, and the default context trumps all, IIRC. There are just too many variables and dimensions of the problem to fix it in .NET Core. I've spent many days focused on this specific problem, and I would get (and ship) solutions that got more or less close to what I wanted, but it failed in too many cases once you introduce different OS's, flavors of Linux, different versions of dotnet.exe.... it was just way too hard to maintain. So now when I have an msbuild Task that includes dependencies that may at all overlap with what MSBuild (core) ships with, I have fallen back to (like you) using a dotnet CLI tool and having my now-simple MSBuild (Core) Task simply invoke that. The problem as I've realized since then is that that too has its issues. Now users have to reference my nuget package and specify a CLI tool in their project explicitly. Also, there are ghostly issues where package restore and invoking my CLI tool mysteriously fails on some machines or in some configurations that I've yet to figure out.

In this scenario, why can't I just hook AppDomain.AssemblyResolve event as a cheap out?

That event handler is only called if the assembly can't be found. If the CLR finds it (but not the version you want) you don't get to intercept it.

[jzabroski]

The problem as I've realized since then is that that too has its issues. Now users have to reference my nuget package and specify a CLI tool in their project explicitly. Also, there are ghostly issues where package restore and invoking my CLI tool mysteriously fails on some machines or in some configurations that I've yet to figure out.

Interesting; thanks for sharing. I guess for the time being my clever workaround will keep my team productive (people aren't upgrading packages and breaking builds as transitive dependencies get updated in NuGet) but has pitfalls.

Because .NET Core doesn't have appdomains. It has AssemblyLoadContext instead, and the default context trumps all, IIRC.

Wow! I skipped .NET Core 1.0 so I had no idea it was removed. But I see you are right:

1. https://stackoverflow.com/a/45600956/1040437
2. http://www.michael-whelan.net/replacing-appdomain-in-dotnet-core/

...and I had technically come across this before, reading Nate McMaster's blog, but not realized what problem he was solving (I assumed he wanted a solution other than/in addition to AppDomains similar to OSGI modules): https://natemcmaster.com/blog/2018/07/25/netcore-plugins/

Interestingly, I was planning to prototype a .NET Core DotNetCli tools extension that was a "plugin of plugins" (a'la MSBuild) called TaskRunner.dll that used Nate's DotNetCorePlugins project as the basis. I just never connected the dots until you just said what you told me.

There are just too many variables and dimensions of the problem to fix it in .NET Core.

This is an interesting argument. I can see that enumerating variables and dimensions would not get us any closer to "solving" the problem, in the sense that if it's too complicated, nobody will be able to actually Know and Follow Rules. However, it doesn't hurt to at least document the variables and dimensions.

[AArnott]

it doesn't hurt to at least document the variables and dimensions.

True. But I never learned all of them (thus my ongoing struggles) and those I learned, I'm afraid I didn't consistently write them down so I don't recall them now. And personally, I'd rather expend effort advocating for a good and proper extensibility model both in .NET Core apps in general, and in MSBuild particularly.

[jzabroski]

But how do you properly advocate if you don't know your user stories? I feel like my original contribution to this thread probably captured about 75% of the user stories, but it's the final 20% that really get you (and another 5% usually get resolved in time for a major release).

For me, I realized in this discussion that the Target Framework Moniker's in a sense add a meta-level to things I had not formally considered, except in this StackOverflow question a year ago: https://stackoverflow.com/questions/51937810/master-list-of-all-nuget-version-to-net-framework-to-assemblyversionattribute-m

As for building something different - I think I already outlined the solution, so if you want to hash it out with me, perhaps offline, I'm interested. See here: dotnet/command-line-api#461 + imagine Nate's DotNetCorePlugins as the glue layer. For things like MSBuild's (poorly designed) Inline Build Tasks, we can use something like the file save format LinqPad 5 uses for defining dependencies.

[AArnott]

But how do you properly advocate if you don't know your user stories?

I'm not sure what I said to give you the impression that I don't know my user stories. I do. But I don't know all the dimensions exist within .NET Core, NuGet, and MSBuild to cause the user stories to fail on some machines.

[jzabroski]

I think I misspoke and it was insulting to you. When I mean user story, I mean it in the most complete sense possible. Consider the following three refinements:

“As a Spotify user, I would like to add songs to a playlist.” Can I break this down further? (Yes).
“As a Spotify user, I would like a menu option to add a song to a playlist.” Specifying the mechanism.
“As a Spotify user, I would like new songs to be added to the bottom of the playlist.” Specifying the policy enacted by the mechanism.

By the way, I mean no insult. I can see how my words could be hurtful and apologize. I'm mainly interested in combining forces and solving what I think is elusively hard problem. I mean, Java literally spent years designing OSGI.

[jzabroski]

Here is another scenario that has bit me: Implicit references fail transitively - In my case, the failure was obvious (compile error) and did not get to the point of needing bindingRedirect. However, I suspect there could be packaging scenarios where authors are silently failing. This might not belong in this thread, but if I don't notate it now, I will surely forget it, as even @AArnott noted that all the combinations make it very hard to keep track of what caused what.

[jmecosta]

here is another scenario that fails for me: dotnet/fsharp#6796

[livarcocc]

We have no plans to implement this for Full Framework MSBuild. We are working on this feature for .NET Core MSBuild using AssemblyLoadContext.

[jzabroski]

@livarcocc Can you please link to an issue specifying the solution using AssemblyLoadContext?

I don't think this will solve the problem fully. 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.