Fix #11025: propagate SkipNonexistentTargets through the metaproj boundary#13650
Conversation
There was a problem hiding this comment.
Pull request overview
Propagates SkipNonexistentTargets into the generated per-project *.metaproj (and related traversal MSBuild invocations) for user-specified targets so solution builds with inter-project dependencies no longer fail with MSB4057 when some projects don’t implement the custom target. The behavior is gated behind the new Wave18_8 change wave for opt-out compatibility.
Changes:
- Add
ChangeWaves.Wave18_8and document it. - Introduce
ShouldSkipNonexistentTargetsForUserTarget(...)and plumb an explicitskipNonexistentTargetsdecision into the metaproj/traversal<MSBuild>task emission paths. - Add unit + end-to-end regression tests for issue #11025.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| documentation/wiki/ChangeWaves.md | Documents the new 18.8 change wave behavior for solution metaprojs. |
| src/Framework/ChangeWaves.cs | Adds Wave18_8 and includes it in the rotating wave list. |
| src/Build/Construction/Solution/SolutionProjectGenerator.cs | Implements the gated propagation of SkipNonexistentTargets to inner <MSBuild> tasks for non-default targets. |
| src/Build.UnitTests/Construction/SolutionProjectGenerator_Tests.cs | Adds regression tests validating the metaproj XML and an end-to-end build scenario. |
There was a problem hiding this comment.
Dimension Review for PR #13650
Dimension 1: Backwards Compatibility — LGTM
Behavioral change is properly gated behind ChangeWave 18.8. Default targets continue to surface MSB4057 on absence (correct). Users can opt out via MSBUILDDISABLEFEATURESFROMVERSION=18.8. No new warnings or errors introduced.
Dimension 2: ChangeWave Discipline — ISSUE
SEVERITY: MAJOR
FILE: src/Build.UnitTests/Construction/SolutionProjectGenerator_Tests.cs
LINES: 2400-2553
SCENARIO: MSBUILDDISABLEFEATURESFROMVERSION=18.8 is set by a user who depends on the strict MSB4057 failure for user targets.
FINDING: No test verifies the disabled path. If a future refactor accidentally removes the ChangeWaves.AreFeaturesEnabled(Wave18_8) guard, no test would catch the regression — users who opted out would silently get new behavior.
RECOMMENDATION: Add a companion test that sets MSBUILDDISABLEFEATURESFROMVERSION=18.8, calls ChangeWaves.ResetStateForTests(), and asserts SkipNonexistentTargets is absent on the "xyz" target's inner MSBuild tasks.
Dimension 3: Performance & Allocation — LGTM
ShouldSkipNonexistentTargetsForUserTarget is called once per project per target during solution project generation (cold path). Uses ImmutableHashSet<string> with OrdinalIgnoreCase for O(1) lookup. No allocations on the call path. Field is static readonly — allocated once.
Dimension 4: Test Coverage — ISSUE
SEVERITY: MAJOR (same as Dimension 2 — the disabled-path test is the gap)
Additional minor observation: the traversal target path (AddTraversalTargetForProject with canBuildDirectly=true) is only implicitly tested via the E2E test. An explicit unit assertion would strengthen coverage but is not blocking.
Dimension 5: Error Message Quality — LGTM
No new error messages or codes. The PR eliminates a false-positive MSB4057 for user targets — correct behavior.
Dimension 6: Logging & Diagnostics — LGTM
No binary log format changes. The SkipNonexistentTargets="true" parameter will be visible in binary logs via the generated project instance, providing adequate diagnostic visibility.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #13650
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - #13650
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13650 · ● 30.7M
…ets through the metaproj boundary The bug is that an `<MSBuild Targets="xyz" SkipNonexistentTargets="true">` task in `Directory.Solution.targets` does not "flow through" to the synthesised per-project `*.metaproj` when the solution has inter-project dependencies. The result is MSB4057 even though the user explicitly opted into target-skipping. Root cause is two-layered: 1. SkipNonexistentTargets is a per-task attribute scoped to the immediate dispatch (engine source: src\Build\BackEnd\...\IntrinsicTasks\MSBuild.cs). It is consumed when the user's task dispatches into the metaproj — and is a no-op there because the metaproj DOES have the target (auto-generated). 2. The metaproj's auto-generated body then makes its own inner <MSBuild> dispatches with their own (generator-provided) parameter set. The user's attribute has no way to reach those inner dispatches because MSBuild never propagates task parameters across dispatch boundaries. The bug class is general: 10 of the 14 MSBuild task parameters silently disappear at the metaproj boundary. SkipNonexistentTargets is one of two that the engine's IntrinsicTasks/MSBuild.cs has no item-metadata or property-based propagation channel for; the other 5 per-item-meaningful parameters (Properties, UndefineProperties, AdditionalProperties, ToolsVersion, SkipNonexistentProjects) already flow through via item metadata. This PR closes the SkipNonexistentTargets gap using the same "engine writes a well-known property; metaproj reads it" pattern. Mechanism: - Engine: `IntrinsicTasks/MSBuild.cs::ExecuteTargets` now sets the global property `_MSBuildInheritedSkipNonexistentTargets=true` on the callee's build request when the dispatching task itself was invoked with `SkipNonexistentTargets="true"` AND the callee path ends in `.metaproj`. Other (non-metaproj) dispatches are unaffected — no global-property churn, no cache-key impact for the common case. - Generator: `SolutionProjectGenerator.cs` synthesises the inner <MSBuild> tasks of metaproj user-target bodies with attribute `SkipNonexistentTargets="$(_MSBuildInheritedSkipNonexistentTargets)"` — a property reference, not a literal "true". The reference evaluates to "true" only when the engine pre-populated it, i.e. only when the user actually opted in. Default targets (Build/Clean/Rebuild/Publish/...) do NOT get the attribute — their absence remains a real authoring bug. Behavior matrix: | User wrote | Outer dispatch type | Result | |----------------------------------|---------------------|-------------------| | SkipNonexistentTargets="true" | -> metaproj | propagates ✓ | | SkipNonexistentTargets="true" | -> non-metaproj | unchanged | | (not set) | -> metaproj | unchanged (MSB4057)| | (not set) | -> non-metaproj | unchanged | Gated behind ChangeWaves.Wave18_8 — opt out via MSBUILDDISABLEFEATURESFROMVERSION=18.8 to restore today's strict failure. Tests (Red -> Green TDD): - Issue11025_MetaprojInnerTasksUseInheritedSkipNonexistentTargetsExpression: XML inspection of generator output. Asserts the property reference is emitted on user-target inner tasks but NOT on default-target tasks. - Issue11025_EngineSetsInheritedPropertyOnMetaprojDispatch: end-to-end against a synthetic .metaproj that echoes the property value. Asserts the engine actually back-propagates. - Issue11025_EngineDoesNotSetInheritedPropertyWhenSkipNonexistentTargetsIsNotTrue: same shape but with the outer task NOT setting the attribute. Asserts legacy behavior preserved (property is empty). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ce8b8d2 to
6138e66
Compare
…kipNonexistentTargets=true The fix only changes behaviour for users who already wrote SkipNonexistentTargets="true" on their outer <MSBuild> task. The only conceivable regression would be a user who wrote that attribute and depended on it being silently ignored at the metaproj boundary -- which is self-contradictory. There is no realistic pre-existing build that relies on the bug, so no ChangeWave gate is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Under the property-reference design, the default-targets exclusion did one of two bad things: 1. No effect, since nobody normally writes <MSBuild Targets='Build' SkipNonexistentTargets='true'> against a sln. 2. Asymmetric refusal to honor the user's attribute when they did write it - silently disregard it for Build/Clean/Rebuild/Publish while honoring it for xyz, violating the 'respect what the user wrote' principle. Apply the property reference uniformly to every synthesised inner <MSBuild> task. Behavior unchanged for users who didn't opt in (property unset -> reference evaluates to empty -> false -> today's MSB4057). The helper is gone; the predicate is gone; the parameter threading through call sites is gone. Expression is one private const at the top of the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Expert Code Review (command) completed successfully! Expert review of PR #13650 completed. The sub-agent (expert-reviewer-pr13650) analyzed the diff and posted inline review comments and a submit_pull_request_review verdict directly to the PR. Key findings included: a gap in SkipNonexistentTargets propagation in AddMetaprojectBuildTask, a duplicate comment introduced on a line in SolutionProjectGenerator.cs, test coverage only for the old sln parser, and a minor allocation cleanup opportunity. No ChangeWave required. All review outputs were submitted by the sub-agent. |
There was a problem hiding this comment.
Review Summary
The design is sound: injecting _MSBuildInheritedSkipNonexistentTargets as a global property on the metaproj callee and having the generated inner tasks read it via a property expression is the right approach. It is backward-compatible by default (the property evaluates to "" = false when the outer caller did not set SkipNonexistentTargets), and the ChangeWave removal is justified because the fix only activates for users who already opted in.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Backwards Compatibility | ✅ LGTM — expression evaluates to empty (false) for all callers who do not set the outer flag; no default behaviour change |
| 2 | ChangeWave | ✅ LGTM — no ChangeWave needed; opt-in only |
| 4 | Test Coverage | 🟡 2 findings (see inline) |
| 22 | Correctness | 🟡 1 finding — AddMetaprojectBuildTask gap (see inline) |
| 20 | Scope | 🔵 NIT — duplicate comment (see inline) |
✅ 20/24 dimensions clean.
Findings requiring action
- [MODERATE – Correctness]
AddMetaprojectBuildTaskdoes not emitSkipNonexistentTargets="$(_MSBuildInheritedSkipNonexistentTargets)". Per-project traversal targets (e.g.Targets="a:xyz") for projects with inter-project dependencies will not benefit from the fix, and MSB4057 can still be raised in that narrower code path. - [MODERATE – Test Coverage] Tests 2 and 3 exercise the engine's injection mechanism against a hand-crafted
.metaproj, not against a real solution build chain. A full end-to-end regression test (real.sln, real project files, missing custom target, verify no MSB4057) is missing. - [NIT – Test Coverage] Test 1 covers only
isOptInSlnParsingWithNewParser: false; the new-parser path should also be exercised. - [NIT – Scope] Duplicate comment
// Add global project referenceon consecutive lines inSolutionProjectGenerator.cs.
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #13650
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (command) for issue #13650 · ● 14.9M
- Remove duplicate // Add global project reference comment - Drop the verbose UnknownProjectType comment block - Extract ForwardInheritedSkipNonexistentTargets helper so the rationale comment lives in one place; simplify the constant docstring to point at the helper - Shorten the MSBuildConstants docstring Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…l e2e test Prior approach (engine back-prop of an inherited global property) was fundamentally broken: changing the metaproj's globals breaks the ConfigCache lookup for the in-memory ProjectInstance registered at sln-load time, so the engine falls back to disk loading and fails with MSB4025. The 'engine end-to-end' test in the prior commit used an on-disk synthetic .metaproj that didn't share that failure mode and so missed it. New design: - Engine: revert the IntrinsicTasks/MSBuild.cs back-prop changes. No engine code change. - Generator: when constructing a metaproject, forward the user-set SolutionMetaprojSkipNonexistentTargets property from solution scope to the metaproject as a *non-global* property via SetProperty. Globals remain unchanged, so the existing ConfigCache lookup still hits the in-memory ProjectInstance. - Generator: every synthesised inner <MSBuild> task continues to emit SkipNonexistentTargets="", which now resolves correctly because the property is in metaproj scope. - User opt-in: set <SolutionMetaprojSkipNonexistentTargets>true</...> in Directory.Solution.props (or any solution-scope property source). When unset, the property reference resolves to empty, the engine treats it as false, and today's MSB4057 behaviour is preserved unchanged. Real Red-Green TDD this time: - Issue11025_BuildSucceedsWhenUserOptsInViaSolutionMetaprojSkipNonexistentTargets: full e2e via RunnerUtilities.ExecMSBuild against the user's reported scenario. Fails on origin/main AND on the prior PR commit. Passes after this redesign. - Issue11025_MetaprojInnerTasksUseSolutionMetaprojSkipNonexistentTargetsExpression: generator XML inspection. Confirms every inner <MSBuild> task on every metaproject target (Build/Clean/Rebuild/Publish/xyz) carries the property reference. - Issue11025_BuildStillFailsWhenUserDidNotOptIn: negative test. Asserts legacy MSB4057 still fires when the opt-in property is not set. Verified all 87 SolutionProjectGenerator_Tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…proj boundary
Final design after the issue reporter shared the literal Directory.Solution.targets
content. Their setup:
<Target Name='xyz'>
<MSBuild Targets='xyz'
BuildInParallel='True'
SkipNonexistentTargets='True'
Projects='@(ProjectReference)'
Properties='BuildingSolutionFile=true; ...SolutionProperties...' />
</Target>
The explicit Properties parameter exactly matches the metaproj's registered
globals, so the dispatch hits the in-memory metaproj cache fine (no MSB4025).
The metaproj's auto-generated xyz target body then dispatches to the
underlying csproj. The user's SkipNonexistentTargets='True' is consumed at
the immediate dispatch (no-op against the metaproj which has the target) and
never reaches the metaproj's own inner dispatches, so they raise MSB4057
when the underlying project doesn't have xyz.
Fix: in RequestBuilder.BuildProjects, when the parent request was for a
metaproject (BuildRequestConfiguration.IsTraversal) and carried the
BuildRequestDataFlags.SkipNonexistentTargets flag, propagate that flag to
the new requests being created. Flags are not part of ConfigurationId, so
the in-memory metaproj cache lookup is unaffected. Restricted to metaproj
contexts so user-authored MSBuild calls don't silently inherit.
This removes the SolutionMetaprojSkipNonexistentTargets opt-in property
approach from the prior commit (which required the user to add a new
property they wouldn't have known about, and didn't actually solve the
literal scenario).
Tests use the issue reporter's literal Directory.Solution.targets content
end-to-end via RunnerUtilities.ExecMSBuild.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two candidate fixes on this branch — reviewer's choiceThe two commits on this branch implement the same end-to-end behavior (same regression tests pass on both) but place the inheritance logic in different layers. Picking one cleanly resolves the PR; the other can be dropped.
What the binlog looks like (side-by-side)Engine-level (commit Task-level (commit Live binlogs comparing the two are checked locally; the propagation message verified to fire at every metaproj-inner dispatch. Tests are identical for bothBoth commits keep the same two
Both fail on RecommendationTask-level (HEAD The engine-level commit is functionally correct but introduces a layer where the operative semantics diverge from what the Task instance reports — which I think is a maintainability tax for anyone who lands on Happy to drop whichever commit you prefer not to keep and force-push a clean single-commit branch. |
|
In what version can we expect it? |
VS 2026 18.8, .NET SDK 10.0.400+ It will not be included in 17.14. Without significant evidence it does not meet the servicing bar. |
Fixes #11025.
Analysis
SkipNonexistentTargetsis a per-task attribute scoped to the immediate dispatch — the engine never copies it onto subsequent dispatches the callee makes. When the callee is a synthesised*.metaproj(a generator-authored wrapper that re-emits<MSBuild>calls to the real underlying projects), the user's intent expressed on the call site that dispatched into the metaproject is consumed there as a no-op (the metaproject has the target — auto-generated) and never reaches the metaproject's own inner dispatches. Those raiseMSB4057when the underlying project lacks the target.flowchart TD U["User xyz target<br/><MSBuild Targets='xyz'<br/>SkipNonexistentTargets='True'<br/>Projects='@(ProjectReference)' />"] -->|"dispatch a.csproj.metaproj /t:xyz<br/>flag: SkipNonexistentTargets"| M["a.csproj.metaproj xyz target<br/>(generator-authored)<br/><MSBuild Targets='xyz' Projects='a.csproj' /><br/>(no SkipNonexistentTargets)"] M -->|"dispatch a.csproj /t:xyz<br/>flag: <b>NOT inherited (bug)</b>"| A["a.csproj<br/>has no xyz target<br/><b>→ MSB4057</b>"] style A fill:#ffcccc,stroke:#cc0000 style M fill:#fff4ccResolution
In
RequestBuilder.BuildProjects, when the parent request was for a metaproject (BuildRequestConfiguration.IsTraversal) and carriedBuildRequestDataFlags.SkipNonexistentTargets, propagate that flag onto the new requests the metaproject's body creates. The metaproject is generator-authored plumbing rather than user code, so propagating the user's intent across that one boundary is the right behaviour. Restricted to metaproject contexts so user-authored<MSBuild>calls do not silently inherit.Crucially, propagation goes via
BuildRequestDataFlags, which does not participate inConfigurationId(BuildRequestConfiguration.cs:1058). The in-memory metaprojectConfigCacheentry continues to match — noMSB4025cache miss, no globals augmentation, no engine-level invention beyond a single conditional flag-OR.flowchart TD U["User xyz target<br/><MSBuild Targets='xyz'<br/>SkipNonexistentTargets='True'<br/>Projects='@(ProjectReference)' />"] -->|"dispatch a.csproj.metaproj /t:xyz<br/>flag: SkipNonexistentTargets"| M["a.csproj.metaproj xyz target<br/>(generator-authored)"] M -->|"dispatch a.csproj /t:xyz<br/>flag: <b>SkipNonexistentTargets ✨ inherited<br/>via RequestBuilder.BuildProjects<br/>when parent IsTraversal</b>"| A["a.csproj<br/>has no xyz target<br/><b>→ silently skipped ✅</b>"] style A fill:#ccffcc,stroke:#009900 style M fill:#fff4cc<MSBuild>taskSkipNonexistentTargets="true"MSB4057The fix is opt-in by the user's existing per-task attribute. No new property, no CLI flag, no auto-injection. No
ChangeWaveneeded: behaviour is unchanged for any caller that didn't set the attribute.Tests
Two
[Fact]s inSolutionProjectGenerator_Tests.cs, both end-to-end viaRunnerUtilities.ExecMSBuildusing the issue reporter's literalDirectory.Solution.targetscontent:Issue11025_UserSkipNonexistentTargetsAttributePropagatesThroughMetaproj— must succeed (fails onorigin/main).Issue11025_BuildStillFailsWhenUserDidNotSetSkipNonexistentTargets— without the attribute,MSB4057must still fire (today's behaviour preserved).Verified red on
origin/main, green with this commit. FullSolutionProjectGenerator_Testsclass: 86/86.