Investigate adding `checkbinarycompat` to check binary incompatibilities

[KirillOsenkov]

It seems like we should be catching regressions such as #10650 at build time, to ensure MSBuild binaries align with NuGet binaries.

Consider using https://www.nuget.org/packages/checkbinarycompat

The readme contains extensive documentation on how to use and how it works, but basically you point it to a directory full of binaries and .app.config files and it checks that for every assembly, type and member reference it actually resolves in the destination assembly, and it checks binding redirect ranges for all entries in the app.config file.

[KirillOsenkov]

I'm debugging the problem with System.Text.Json.dll and I'm not sure I understand what's going on.

I'm seeing a bunch of .dlls loaded twice:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.ProjectModel.dll
C:\Program Files\dotnet\sdk\9.0.100-rc.1.24452.12\Sdks\Microsoft.NET.Sdk\tools\net472\NuGet.ProjectModel.dll

The one in the SDK indeed references 8.0.0.4. The one in VS references 7.0.0.3.

Apparently there's also the NuGetFrameworkWrapper appdomain? But both of the above .dlls are in the default appdomain?

And NuGet.Frameworks.dll is loaded three times? 1 from SDK and 1 from VS in the primary appdomain and 1 from VS in the NuGet appdomain?

I feel like this is a recipe for disaster. We need to understand what is going on here:

[KirillOsenkov]

As for checkbinarycompat, we should determine which .dlls are getting loaded into MSBuild.exe at runtime and from which locations, copy them all into a temp folder, then copy MSBuild.exe.config into that folder, then run checkbinarycompat on that folder. This will ensure that the actual combination of the assemblies loaded together at runtime is being tested together.

[KirillOsenkov]

But honestly the more important task I think is to ensure dlls are not double-loaded, so probably needs another AssemblyResolve handler similar to MSBuild Locator that when the SDK requests a dll it gets loaded from the VS location (or the other way around)

cc @rainersigwald @baronfel

[rainersigwald]

But honestly the more important task I think is to ensure dlls are not double-loaded

I don't think I agree and in fact would treat this as a non-goal. From a functional perspective it's ok for tasks to bring their own, higher copies of assemblies that VS/the SDK provide, as long as no reference requires a binding redirect. In addition, the engine may choose to play tricks (like NuGetFrameworkWrapper) to reduce runtime costs.

[KirillOsenkov]

Hmm, so are you saying the double loading is OK? Can we envision a setup where we only load NuGet assemblies from a single place? I know for a fact that double loading is a world of risk, but if we're getting lucky normally and you're OK with living dangerously that's fine by me, of course.

Just surprised nobody is screaming and panicking at the sight of double loaded assemblies. Maybe it's just me and my scars. I've seen things you people wouldn't believe.

[KirillOsenkov]

also, AppDomains, yuck

[rainersigwald]

Yeah we're definitely aiming for least-worst here.

Can we envision a setup where we only load NuGet assemblies from a single place?

Probably not, I can't really see a way to isolate VS and the SDK from each other while preserving this.

[KirillOsenkov]

OK, then this issue is still about looking at whether we can do checkbinarycompat to catch these issues earlier (Shift Left baby!)

[AR-May]

Team triage: We need to manually verify dotnet 9 GA sdk with high prio. With lower prio we need to automate the check.

[JanKrivanek]

(thought by @rainersigwald:)

In scope of this task we want to check closure of msbuild and tasks shipped.

For future we we should think about facilitating automation of that control for custom authored tasks

[ViktorHofer]

Note that the .NET SDK has APICompat for that which AFAIK is already enabled in dotnet/msbuild. That tool checks api compatibility and allows certain changes that aren't source and/or binary breaking.

[rainersigwald]

@ViktorHofer APICompat is insufficient for our needs--this is guarding against a break that wasn't a "breaking" change anywhere but broke users anyway.