Investigate adding `checkbinarycompat` to check binary incompatibilities

[KirillOsenkov]

It seems like we should be catching regressions such as https://github.com/dotnet/msbuild/pull/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.

[GangWang01]

With bootstrapped core(\artifacts\bin\bootstrap\core) after executing build.cmd using latest main that specifies dotnet 9.0.100-rc.2.24474.11, I did the check with the tool checkbinarycompat.

The tool has different behaviors if msbuild-related file specs are different. See Reports.zip

• check the bootstrap core directory with command checkbinarycompat -s -l: The check doesn't find dependencies' version mismatch for msbuild assemblies which could be found using the pattern **\Microsoft.Build*.dll below for file specs. The version mismatch looks like " Assembly `Microsoft.Build.Tasks.Core` is referencing `System.CodeDom, Version=8.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` but found `System.CodeDom, Version=9.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` at `sdk\9.0.100-rc.2.24474.11\System.CodeDom.dll`"
• check msbuild assemblies with command checkbinarycompat **\Microsoft.Build*.dll -s -l: Though it could find some dependencies' version mismatch, some are not correct. For example, "Assembly `Microsoft.Build.Framework` is referencing `System.Collections.Immutable, Version=9.0.0.0, PublicKeyToken=b03f5f7f11d50a3a` but found `System.Collections.Immutable, Version=8.0.0.0, PublicKeyToken=b03f5f7f11d50a3a` at `sdk\9.0.100-rc.2.24474.11\DotnetTools\dotnet-format\BuildHost-net472\System.Collections.Immutable.dll`", actually msbuild is referencing System.Collections.Immutable 8.0.0 currently. However, we can take it as reference reminding to upgrade the dependency.
• check msbuild.dll/msbuild.dll.config with command checkbinarycompat **\MSBuild.dll* -s -l: No msbuild.dll issue found and this is consistent with checking the directory. For app.config MSBuild.dll.config, checking the directory has the correct report.

Note that why I didn't used semicolon-separated patterns for file specs, it's because checkbinarycompat **\Microsoft.Build*.dll;**\MSBuild.dll* -s -l reports no issue found. I think this is a bug for the tool.

ReportAsReference.txt is combined report as reference.

[KirillOsenkov]

A couple of tips for using checkbinarycompat.

First, make a response file with the list of directories and binaries that you'd like to scan together.

For example make a file called binarycompat.rsp:

#.dotnet\sdk\*\Sdks\Microsoft.Build.Tasks.Git\tools\net472\Microsoft.Build.Tasks.Git.dll
#.dotnet\sdk\*\Sdks\Microsoft.NET.Sdk\tools\net472
#!.dotnet\sdk\*\Sdks\Microsoft.NET.Sdk\tools\net472\NuGet*.dll
#.dotnet\sdk\*\Sdks\Microsoft.SourceLink.*\tools\net472
artifacts\bin\bootstrap\net472\Common7\IDE\CommonExtensions\Microsoft\NuGet
artifacts\bin\bootstrap\net472\MSBuild\Current\Bin
artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\amd64\MSBuild.exe
artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\Roslyn\Microsoft.Build.Tasks.CodeAnalysis.dll
artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\SdkResolvers\**\*.dll
#C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet

You can use # as a comment and ! as an exclude filter. Then just run using:

checkbinarycompat @binarycompat.rsp | clip

It will pipe the output of the tool to the clipboard, you can then paste into any text editor to see where the tool picked up each .dll from.

With the rsp file as above and the latest build of MSBuild main branch I get this report:

App.config: 'MSBuild.exe.config': couldn't find assembly 'Microsoft.Build.Conversion.Core' with version 15.1.0.0.
App.config: 'MSBuild.exe.config': couldn't find assembly 'Microsoft.Build.Engine' with version 15.1.0.0.
App.config: 'MSBuild.exe.config': couldn't find assembly 'Microsoft.NET.StringTools.net35' with version 1.0.0.0.
App.config: 'MSBuild.exe.config': couldn't find assembly 'System.ValueTuple' with version 4.0.0.0. Found versions: 4.0.3.0
App.config: 'MSBuild.exe.config': couldn't find assembly 'XamlBuildTask' with version 17.0.0.0.
App.config: 'MSBuild.exe.config': dependentAssembly for FxCopTask doesn't have a bindingRedirect subelement
App.config: 'MSBuild.exe.config': dependentAssembly for Microsoft.Deployment.DotNet.Releases doesn't have a bindingRedirect subelement
App.config: 'MSBuild.exe.config': dependentAssembly for Microsoft.DotNet.MSBuildSdkResolver doesn't have a bindingRedirect subelement
App.config: 'MSBuild.exe.config': dependentAssembly for Microsoft.VisualStudio.CodeAnalysis doesn't have a bindingRedirect subelement
App.config: 'MSBuild.exe.config': dependentAssembly for Microsoft.VisualStudio.CodeAnalysis.Sdk doesn't have a bindingRedirect subelement
App.config: 'MSBuild.exe.config': oldVersion range for Microsoft.Activities.Build is in incorrect format
Assembly `BuildXL.Utilities.Core` is referencing `System.Threading.Channels, Version=7.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` but found `System.Threading.Channels, Version=8.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` at `artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\System.Threading.Channels.dll`
In assembly 'NuGet.Build.Tasks, Version=6.12.0.127, PublicKeyToken=31bf3856ad364e35': Failed to resolve assembly reference to 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0, PublicKeyToken=b03f5f7f11d50a3a'
In assembly 'NuGet.Build.Tasks.Console, Version=6.12.0.127, PublicKeyToken=31bf3856ad364e35': Failed to resolve assembly reference to 'Microsoft.Build.Utilities.v4.0, Version=4.0.0.0, PublicKeyToken=b03f5f7f11d50a3a'

You can have several rsp files, each for a testing scenario that you care about. I recommend picking directories together to mimic the exact set of .dlls loaded at runtime. For this you can run devenv.exe or msbuild.exe under debugger, and view the Debug -> Windows -> Modules tool window for the list of full paths loaded into the process, like in my screenshot above.

To ensure it will actually catch real issues we need to reproduce the exact configuration that resulted in https://github.com/dotnet/msbuild/pull/10650 and ensure we get the warning about the wrong System.Text.Json binding redirect.

When you say there's a bug in the tool, I'm going to need proof: a zip file with a minimal repro: just the absolute minimal number assemblies that when scanned together result in a different report that you expect. I doubt there's a bug, but I've been wrong before.

[KirillOsenkov]

Also I recommend several command line tools to aid in these types of investigations:

https://nuget.org/packages/lbi https://nuget.org/packages/refdump https://nuget.org/packages/ff https://nuget.org/packages/checkbinarycompat

Each of them is a dotnet global tool, so you can install using for example dotnet tool update -g lbi.

lbi

lbi finds and groups assemblies in a directory recursively:

refdump

refdump finds all assemblies that reference a given assembly:

ff

ff is just a fast file finder

[GangWang01]

Thanks @KirillOsenkov for the tips!

We need to check with dotnet 9 sdk. artifacts\bin\bootstrap\core is the target directory which has newly built msbuild assemblies and config file copied into installed dotnet sdk there. With this response file, the report BinaryCompatReport_MSBuildFiles.txt looks better.

core\sdk\**\Microsoft.Build*.dll
core\sdk\**\MSBuild.dll
core\sdk\**\MSBuild.dll.config

For checking all the assemblies and config files under artifacts\bin\bootstrap\core using the response file below, it could not report the dependencies' version mismatch for msbuild assemblies, for example "Assembly `Microsoft.Build.Tasks.Core` is referencing `System.CodeDom, Version=8.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` but found `System.CodeDom, Version=9.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` at `core\sdk\9.0.100-rc.2.24474.11\System.CodeDom.dll`". See BinaryCompatReport_dotnet.txt.

core\**\*.dll
core\**\*.config

[GangWang01]

With this response file, the report BinaryCompatReport_MSBuildFiles.txt looks better.

core\sdk\**\Microsoft.Build*.dll
core\sdk\**\MSBuild.dll
core\sdk\**\MSBuild.dll.config

I plan to upgrade the versions according to the report and run the tool checkbinarycompat to check again. Unfortunately, internal nuget feeds hasn't have the latest versions yet. To work around I tried to add nuget.org feed https://api.nuget.org/v3/index.json and updated System.CodeDom from 8.0.0 to 9.0.0-rc.2.24473.5 which is currently latest but no released 9.0.0. Build succeeded. But running the tool to check the report still had "Assembly `Microsoft.Build.Tasks.Core` is referencing `System.CodeDom, Version=8.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` but found `System.CodeDom, Version=9.0.0.0, PublicKeyToken=cc7b13ffcd2ddd51` at `core\sdk\9.0.100-rc.2.24474.11\System.CodeDom.dll`".

[rainersigwald]

Thanks @GangWang01! I think we're good for .NET 9. We should continue to investigate how to push this knowledge to our users for arbitrary task creation.