Skip to content

MSBuildBinaryLog: Retrieve inner assets files for multi-target project - #1891

Open
Julien Lebosquain (MrJul) wants to merge 4 commits into
microsoft:mainfrom
MrJul:fix/multi-target-assets
Open

Julien Lebosquain (MrJul) wants to merge 4 commits into
microsoft:mainfrom
MrJul:fix/multi-target-assets

Conversation

@MrJul

Copy link
Copy Markdown

Problem

When using the MSBuildBinaryLog detector with a multi-target project, any information from the project, such as <IsShipping>false</IsShipping> isn't applied and a warning is logged:

No ProjectAssetsFile property found in binlog for project [...]

This is because the outer builder doesn't really have an asset file, each inner build does (even though it's shared).

Solution

This PR now iterates through all inner builds to map the assets file to its matching project.

Two unit tests, which were failing before the changes, have been added.

@MrJul

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Multi-binlog merging can discard inner-build metadata, and the behavior change requires a detector version bump.

2 open findings
What changed in this PR

Enables MSBuild binlog detection for multi-target projects whose assets files are recorded only by inner builds.

Changes:

  • Indexes and validates inner-build assets files.
  • Adds multi-target regression tests for development dependencies.
File Description
MSBuildBinaryLogComponentDetector.cs Retrieves assets paths from inner builds.
MSBuildBinaryLogComponentDetectorTests.cs Tests inner-build lookup and dependency classification.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

// (e.g., build and publish passes) form a superset rather than keeping only the first.
// Normalize to forward slashes so lookup from OS-native ComponentStream.Location matches.
if (!string.IsNullOrEmpty(projectInfo.ProjectAssetsFile))
foreach (var assetsFile in GetProjectAssetsFiles(projectInfo))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ac03948 now merges the inner builds.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 14:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Inner-build merging can incorrectly discard IsShipping=false, causing dependency misclassification.

1 open finding
1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity MergeWith incorrectly prefers true for inverted IsShipping flag

src/​Microsoft.ComponentDetection.Detectors/​nuget/​MSBuildProjectInfo.cs:284

The recursive merge can discard a development-only classification. MergeWith merges every nullable boolean with “true wins” (MSBuildProjectInfo.cs:257-262), but IsShipping has inverted semantics: false marks all dependencies as development dependencies (MSBuildBinaryLogComponentDetector.cs:201-204, also documented in docs/detectors/nuget.md:52). If one binlog reports an inner build as shipping and a later build/publish binlog reports IsShipping=false, this call retains true, so the dependencies are incorrectly classified as shipping. Merge IsShipping with false-wins semantics and add a conflicting multi-binlog inner-build regression test.

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants