add MSBuild binary log (.binlog) component detector #1250

brettfo · 2024-09-19T18:18:40Z

Adds a new NuGet component detector to scan MSBuild binary log (*.binlog) files.

There was one major reason to add this: the current NuGet component detector that scans project.assets.json isn't always 100% accurate. This can happen when a given NuGet package has a transitive dependency on a System.* package, but the installed SDK has a newer version of that file that is replaced at build time.

One common example: a project has <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="6.0.0" />. That package has a transitive dependency on System.Text.Json/6.0.0. No matter the SDK used to restore and build the project, the project.assets.json file will report that the package System.Text.Json/6.0.0 was used and implies that it'll be present at runtime. However, if the 6.0.424 SDK or newer was used to build, the System.Text.Json.dll file from the 6.0.0 package is removed and replaced with a newer System.Text.Json.dll from the SDK's runtime package.

To avoid reporting a false positive for a package that was replaced at build time (and therefore won't be present at runtime) the MSBuild binary log scanner was added.

If a binary log is generated with the /bl flag, every MSBuild event will be recorded. The new detector in this PR scans each event for the relevant AddItem and RemoveItem elements (corresponding to things like <SomeItemGroup Include="..." ... /> and <SomeItemGroup Remove="..." ... /> and uses that to build a 100% accurate dependency set.

Most of the content of this PR is to support the tests to make them as reliable as possible through 2 different means:

Generate a fresh .binlog file for each test by running the appropriate dotnet build ... command. This ensures there are no large binary test assets added to this repo; they're simply generated as needed.
Mock all necessary NuGet packages to prevent network access. To accomplish this, 3 common NuGet packages are faked for the tests: Microsoft.NETCore.App.Ref, Microsoft.AspNetCore.App.Ref, and Microsoft.WindowsDesktop.App.Ref.

Future work

Track packages implicitly added by the SDK when publishing an app. In a quick local test, the package Microsoft.NETCore.App.Host.win-x64 was added to the following item groups:

AppHostPack
SingleFileHostPack
ComHostPack
IjwHostPack
ResolvedAppHostPack
ResolvedSingleFileHostPack
ResolvedComHostPack
ResolvedJjwHostPack

To fully prevent reporting false positives, it would be a good idea to merge the binary log detector and the project.assets.json detector so that if a binary log is present and covers a given .csproj, only the binary log detector is used and fall back to the original detector otherwise. Without this work, the binary log detector will properly not report System.Text.Json/6.0.0, but the project.assets.json detector would, so the false positive will still appear.

codecov · 2024-09-19T18:46:15Z

Codecov Report

Attention: Patch coverage is 95.35284% with 27 lines in your changes missing coverage. Please review.

Project coverage is 89.0%. Comparing base (8360853) to head (eee567f).

Files with missing lines	Patch %	Lines
...tDetection.Detectors.Tests/MSBuildTestUtilities.cs	93.5%	11 Missing and 4 partials ⚠️
...rs/nuget/NuGetMSBuildBinaryLogComponentDetector.cs	94.5%	7 Missing and 2 partials ⚠️
...sts/NuGetMSBuildBinaryLogComponentDetectorTests.cs	98.2%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1250     +/-   ##
=======================================
+ Coverage   88.9%   89.0%   +0.1%     
=======================================
  Files        359     362      +3     
  Lines      27672   28250    +578     
  Branches    1784    1836     +52     
=======================================
+ Hits       24613   25165    +552     
- Misses      2674    2695     +21     
- Partials     385     390      +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

KirillOsenkov · 2024-09-19T19:26:05Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+ if (!topLevelDependencies.TryGetValue(projectEvaluation.ProjectFile, out var topLevel))
+ {
+ topLevel = new(StringComparer.OrdinalIgnoreCase);
+ topLevelDependencies[projectEvaluation.ProjectFile] = topLevel;


one project file may be evaluated and built several times, you'll often see several evaluations per .csproj. Should we pick the best evaluation somehow or is it fine to do for each evaluation? Restore does an eval, then the build does another eval, and each target framework is a separate eval. We probably want a union of all evaluations?

I'll experiment a bit to see if I can find inaccurate results, but generally we really only care about the item groups that NuGet populates like @(RuntimeCopyLocalItems) and I think that only happens during the Restore evaluation.

The restore evaluation is going to be pretty useless (it gets packages downloaded but doesn't produce any of the related items). You will definitely need to do all of the inner-build evaluations so that you get the superset of references, e.g.

<Project Sdk="Microsoft.NET.Sdk"> <PropertyGroup> <TargetFrameworks>net8.0;net472</TargetFrameworks> </PropertyGroup> <ItemGroup> <PackageReference Include="Newtonsoft.Json" Version="13.0.3" Condition=" '$(TargetFramework)' == 'net8.0' " /> <PackageReference Include="System.Text.Json" Version="8.0.4" Condition=" '$(TargetFramework)' == 'net472' " /> </ItemGroup> </Project>

KirillOsenkov · 2024-09-19T19:31:15Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+ topLevelDependencies[projectEvaluation.ProjectFile] = topLevel;
+ }
+
+ if (doRemoveOperation)


a bit worried that you might be getting additem/removeitem from different evaluations in an interleaved way, and since they key by the project path this might get confused.

On the other hand it shouldn't happen because the binlog is a tree, and you're walking the tree linearly, so in theory each evaluation should be processed sequentially one after another.

What might be an indication that the traversal is bad? If I try to process RemoveItem but the item isn't already present? Or is that something that MSBuild won't allow?

yes I guess, but as I said, maybe I'm just being paranoid

KirillOsenkov · 2024-09-19T19:43:30Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+ projectDependencies[packageName] = packageVersion;
+ }
+
+ project = project.GetNearestParent<Project>();


hmm, why are you walking up the project chain? I don't think these items flow to the calling project?

I found I needed this to track transitive dependencies. E.g., if library.csproj has <PackageReference Include="Some.Package" /> and unitTests.csproj has <ProjectReference Include="..\library\library.csproj" /> this will add Some.Package first to library.csproj and then crawl up the chain to unitTests.csproj so that the dependency is properly reported for both projects. The only oddity with this (and I may have to special case it) is the .sln file also appears in a Project node, but I do seem to be getting the proper hierarchy.

I don't think I understand this. unitTests.csproj should have Some.Package in its assets file and pull assets out of it in its ResolvePackageAssets. Can you share a log or project setup where this is necessary?

KirillOsenkov · 2024-09-19T19:45:46Z

As an alternative, you could also see how binlogtool listnuget msbuild.binlog output.txt does it. It opens all embedded project.assets.json files, reads the contents into a model, and walks all of them to collect a flat list of all packages and versions used by the build.

I think your approach is fine too (?)

@jeffkl as an expert FYI, in case he wants to take a quick look

KirillOsenkov · 2024-09-19T19:46:30Z

actually we could diff the results of binlogtool listnuget with these results, and see if there are any discrepancies. Would be like an oracle!

brettfo · 2024-09-19T21:39:31Z

As an alternative, you could also see how binlogtool listnuget msbuild.binlog output.txt does it. It opens all embedded project.assets.json files, reads the contents into a model, and walks all of them to collect a flat list of all packages and versions used by the build.

I think your approach is fine too (?)

@jeffkl as an expert FYI, in case he wants to take a quick look

I explicitly don't want the contents of project.assets.json because they don't necessarily reflect what ends up in the output. There are more details in the original description, but in some cases the SDK will replace an assembly that NuGet has already resolved, so according to NuGet, a certain package ended up in the build, but in reality it's not there and shouldn't get reported.

KirillOsenkov · 2024-09-19T22:32:49Z

I see, makes sense! 👍🏻

brettfo · 2024-09-19T22:48:16Z

For those interested, I ran the package Microsoft.Extensions.Configuration.Json/6.0.0 through both the binlogtool suggested above and this PR. The results are as follows:

binlogtool:

Microsoft.Extensions.Configuration/6.0.0
Microsoft.Extensions.Configuration.Abstractions/6.0.0
Microsoft.Extensions.Configuration.FileExtensions/6.0.0
Microsoft.Extensions.Configuration.Json/6.0.0
Microsoft.Extensions.FileProviders.Abstractions/6.0.0
Microsoft.Extensions.FileProviders.Physical/6.0.0
Microsoft.Extensions.FileSystemGlobbing/6.0.0
Microsoft.Extensions.Primitives/6.0.0
System.Runtime.CompilerServices.Unsafe/6.0.0
System.Text.Encodings.Web/6.0.0
System.Text.Json/6.0.0

This PR:

Microsoft.Extensions.Configuration/6.0.0
Microsoft.Extensions.Configuration.Abstractions/6.0.0
Microsoft.Extensions.Configuration.FileExtensions/6.0.0
Microsoft.Extensions.Configuration.Json/6.0.0
Microsoft.Extensions.FileProviders.Abstractions/6.0.0
Microsoft.Extensions.FileProviders.Physical/6.0.0
Microsoft.Extensions.FileSystemGlobbing/6.0.0
Microsoft.Extensions.Primitives/6.0.0

The difference being that the SDK (6.0.425 on my machine) removed the System.* packages.

rainersigwald · 2024-09-20T14:43:14Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+ if (!topLevelDependencies.TryGetValue(projectEvaluation.ProjectFile, out var topLevel))
+ {
+ topLevel = new(StringComparer.OrdinalIgnoreCase);
+ topLevelDependencies[projectEvaluation.ProjectFile] = topLevel;


The restore evaluation is going to be pretty useless (it gets packages downloaded but doesn't produce any of the related items). You will definitely need to do all of the inner-build evaluations so that you get the superset of references, e.g.

<Project Sdk="Microsoft.NET.Sdk"> <PropertyGroup> <TargetFrameworks>net8.0;net472</TargetFrameworks> </PropertyGroup> <ItemGroup> <PackageReference Include="Newtonsoft.Json" Version="13.0.3" Condition=" '$(TargetFramework)' == 'net8.0' " /> <PackageReference Include="System.Text.Json" Version="8.0.4" Condition=" '$(TargetFramework)' == 'net472' " /> </ItemGroup> </Project>

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

rainersigwald · 2024-09-20T16:23:04Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+
+ if (doRemoveOperation)
+ {
+ topLevel.Remove(packageName);


Specifically I think this is going to cause problems if one TF of a project references a thing and the other gets it removed by framework unification.

Do you have an example of this? I'd like to add a test to make sure we don't remove anything that shouldn't be removed.

rainersigwald · 2024-09-20T16:27:40Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs

+ projectDependencies[packageName] = packageVersion;
+ }
+
+ project = project.GetNearestParent<Project>();


I don't think I understand this. unitTests.csproj should have Some.Package in its assets file and pull assets out of it in its ResolvePackageAssets. Can you share a log or project setup where this is necessary?

rainersigwald · 2024-09-20T16:38:02Z

Directory.Packages.props

 <PackageVersion Include="MSTest.TestFramework" Version="3.5.1" />
 <PackageVersion Include="MSTest.Analyzers" Version="3.5.1" />
 <PackageVersion Include="MSTest.TestAdapter" Version="3.5.1" />
+ <PackageVersion Include="Microsoft.Build.Framework" Version="17.5.0" />
+ <PackageVersion Include="Microsoft.Build.Locator" Version="1.6.1" />


I have a concern here. Are you running as a standalone .NET 6 application currently? If so, you won't be able to load MSBuild from new SDKs. Have you tried on a machine that has only .NET 8 installed?

This seems to work in the unit tests which should only have .NET6 installed (it's installing here and if I understand that action correctly, it uses global.json to determine what to install, so no .NET 8 on the CI machine)

Right, the problem will be when you run the tool on a different machine which has only .NET 8 installed.

brettfo force-pushed the msbuild-binlog branch 3 times, most recently from 99ffda7 to 2f8d76a Compare September 19, 2024 18:40

add MSBuild binary log (.binlog) component detector

135fdd2

brettfo force-pushed the msbuild-binlog branch from 2f8d76a to 135fdd2 Compare September 19, 2024 18:43

KirillOsenkov reviewed Sep 19, 2024

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs Show resolved Hide resolved

KirillOsenkov reviewed Sep 19, 2024

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetMSBuildBinaryLogComponentDetector.cs Outdated Show resolved Hide resolved

KirillOsenkov reviewed Sep 19, 2024

View reviewed changes

brettfo added 2 commits September 19, 2024 16:35

add lock around MSBuild registration

8b4aa9c

add test for .sln with two unrelated projects

910caa4

brettfo force-pushed the msbuild-binlog branch from 1ce5859 to 910caa4 Compare September 19, 2024 22:35

don't report dependencies from .sln files

eee567f

rainersigwald reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add MSBuild binary log (.binlog) component detector #1250

add MSBuild binary log (.binlog) component detector #1250

brettfo commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

KirillOsenkov Sep 19, 2024

brettfo Sep 19, 2024

rainersigwald Sep 20, 2024

KirillOsenkov Sep 19, 2024

brettfo Sep 19, 2024

KirillOsenkov Sep 19, 2024

KirillOsenkov Sep 19, 2024

brettfo Sep 19, 2024

rainersigwald Sep 20, 2024

KirillOsenkov commented Sep 19, 2024

KirillOsenkov commented Sep 19, 2024

brettfo commented Sep 19, 2024

KirillOsenkov commented Sep 19, 2024

brettfo commented Sep 19, 2024 •

edited

Loading

rainersigwald Sep 20, 2024

rainersigwald Sep 20, 2024

brettfo Sep 20, 2024

rainersigwald Sep 20, 2024

rainersigwald Sep 20, 2024

brettfo Sep 20, 2024

rainersigwald Sep 20, 2024

add MSBuild binary log (.binlog) component detector #1250

Are you sure you want to change the base?

add MSBuild binary log (.binlog) component detector #1250

Conversation

brettfo commented Sep 19, 2024 • edited Loading

Future work

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirillOsenkov commented Sep 19, 2024

KirillOsenkov commented Sep 19, 2024

brettfo commented Sep 19, 2024

KirillOsenkov commented Sep 19, 2024

brettfo commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettfo commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

brettfo commented Sep 19, 2024 •

edited

Loading