Add warnings and guidance against TargetFramework centralization#303
Add warnings and guidance against TargetFramework centralization#303MattKotsenas wants to merge 5 commits intodotnet:mainfrom
Conversation
…ry.Build.props The existing AP-21 guidance covered conditioning on in .props files, but nothing warned against setting <TargetFramework> or <TargetFrameworks> itself in Directory.Build.props as a 'default'. When .props sets the singular and a project sets the plural (or vice versa), both properties coexist and MSBuild simultaneously runs inner and outer builds, breaking everything. Added callouts in all places where existing guidance could be misinterpreted to include TFM in centralization: - targetframework-props-pitfall.md: new top section explaining the inner/outer build collision - directory-build-organization SKILL.md: 'Do NOT put here' now explicitly names TargetFramework/TargetFrameworks; workflow step 6 carves out TFM from 'remove all centralized properties' - msbuild-antipatterns AP-08: exception note that TFM must not be centralized even when repeated - multi-level-examples.md: comment in Before/After explaining why TFM was deliberately not centralized - msbuild-code-review agent and msbuild-pr-review workflow: added TFM centralization as a check item Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Revert AP-08 exception note (overkill) - Remove hyperlinks from SKILL.md callouts that weren't there before - Move centralization pitfall content from targetframework-props-pitfall.md to its own file: targetframework-centralization-pitfall.md - Add DefaultTargetFramework workaround for centralizing the version string without centralizing the property itself Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Assign AP-22 to the TargetFramework/TargetFrameworks in Directory.Build.props antipattern so it follows the same numbered convention as the rest of the catalog. - Added AP-22 entry in additional-antipatterns.md with full explanation and DefaultTargetFramework workaround - Added AP-22 to the quick-reference checklist (severity: build collision) - Updated msbuild-pr-review.md to use AP-22 prefix - Updated SKILL.md range reference to AP-16 through AP-22 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delete targetframework-centralization-pitfall.md — the AP-22 entry in additional-antipatterns.md already covers the same content. Update references in multi-level-examples.md and msbuild-code-review agent to cite AP-22 directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/cc @baronfel, as I think I've mentioned that people commonly do this when they first learn about D.B.p |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MSBuild antipattern entry (AP-22) warning against setting <TargetFramework> or <TargetFrameworks> in Directory.Build.props, which can cause inner/outer build collisions when a project file sets the other variant. The changes are spread across skill documentation, agent instructions, and compiled lock files to ensure consistent guidance across the repository.
Changes:
- Added AP-22 with full explanation, BAD/GOOD examples, and a safe
DefaultTargetFrameworkworkaround to the antipattern catalog and quick-reference checklist - Hardened existing guidance in
directory-build-organizationskill (SKILL.md, multi-level-examples.md) to explicitly warn against centralizing TFM properties - Added AP-22 detection to the MSBuild code review agent and PR review workflow, and regenerated compiled lock files
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/dotnet-msbuild/skills/msbuild-antipatterns/references/additional-antipatterns.md |
Added AP-22 antipattern definition and checklist entry |
plugins/dotnet-msbuild/skills/msbuild-antipatterns/SKILL.md |
Updated reference range to include AP-22 |
plugins/dotnet-msbuild/skills/directory-build-organization/SKILL.md |
Explicitly named TargetFramework in "Do NOT put here" list and workflow step 6 |
plugins/dotnet-msbuild/skills/directory-build-organization/references/multi-level-examples.md |
Added note and XML comment explaining why TFM is not centralized in examples |
plugins/dotnet-msbuild/agents/msbuild-code-review.agent.md |
Added AP-22 detection check to code review agent |
agentic-workflows/dotnet-msbuild/msbuild-pr-review.md |
Added AP-22 detection check to PR review workflow |
agentic-workflows/dotnet-msbuild/shared/compiled/style-and-modernization.lock.md |
Regenerated lock file with AP-22 content (has corruption issues) |
agentic-workflows/dotnet-msbuild/shared/compiled/performance.lock.md |
Fixed incorrect skill reference (analyzer-performance → build-perf-diagnostics) |
agentic-workflows/dotnet-msbuild/shared/compiled/build-errors.lock.md |
BOM removal and dash style update in header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| For additional anti-patterns (AP-16 through AP-22) and a quick-reference checklist, see ## AP-16: Using `<Exec>` for String/Path Operations | ||
|
|
There was a problem hiding this comment.
This line is corrupted — the reference text from SKILL.md line 387 (For additional anti-patterns (AP-16 through AP-22) and a quick-reference checklist, see [additional-antipatterns.md](...)) has been incorrectly merged with the AP-16 heading that should follow on a separate line. This should be two separate lines: the reference paragraph (with proper link formatting), followed by a blank line and the ## AP-16 heading.
| For additional anti-patterns (AP-16 through AP-22) and a quick-reference checklist, see ## AP-16: Using `<Exec>` for String/Path Operations | |
| For additional anti-patterns (AP-16 through AP-22) and a quick-reference checklist, see [additional-antipatterns.md](...). | |
| ## AP-16: Using `<Exec>` for String/Path Operations |
| <Project Sdk="Mi | ||
|
|
||
| [truncated] No newline at end of file |
There was a problem hiding this comment.
The file is truncated mid-content — the deletion removed the rest of the WPF/WinForms example, the test project section, the Central Package Management migration section, and the Directory.Build Consolidation section. The file now ends with a partial XML tag (<Project Sdk="Mi) inside an unclosed code fence, followed by a literal [truncated] marker that doesn't exist in other lock files.
While removing the old Directory.Build Consolidation example that showed <TargetFramework> in Directory.Build.props is aligned with AP-22, the other deleted sections (WindowsDesktop SDK usage, test project examples, CPM migration) appear to be unrelated and useful content that should be preserved. The file should be re-compiled properly to include the AP-22 additions without losing unrelated content.
…ernization.lock.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | AP-17 | Mixed Include/Update in one ItemGroup | 🔵 Subtle bugs | | ||
| | AP-18 | Redundant transitive ProjectReferences | 🔵 Graph noise | | ||
| | AP-20 | Platform-specific Exec without guard | 🔵 Cross-platform | | ||
| | AP-22 | TargetFramework/TargetFrameworks in Directory.Build.props | 🔴 Build collision | |
There was a problem hiding this comment.
AP-22 has 🔴 severity but is placed at the bottom of the checklist table, after all the 🟡 and 🔵 items. The table header says "scan for these in order" and groups the other 🔴 items at the top (AP-02, AP-19, AP-21, AP-03). AP-22 should be placed alongside the other 🔴 entries (e.g., after AP-03 on line 235) to maintain the intended severity-based scan order.
| ## AP-22: Setting `<TargetFramework>` or `<TargetFrameworks>` in Directory.Build.props | ||
|
|
||
| **Smell**: `<TargetFramework>` or `<TargetFrameworks>` set in `Directory.Build.props` (at any level) as a "default" TFM. | ||
|
|
||
| **Why it's bad**: If `Directory.Build.props` sets the singular `<TargetFramework>` and a project sets the plural `<TargetFrameworks>` (or vice versa), both properties coexist. MSBuild interprets `<TargetFrameworks>` (plural) as a request for an outer build that dispatches inner builds per TFM, while also seeing `<TargetFramework>` (singular) which signals an inner build. The build is simultaneously an inner and outer build — targets run in the wrong order, items appear in the wrong phase, and the build produces corrupt or nonsensical results. |
There was a problem hiding this comment.
The msbuild-modernization skill's "Directory.Build Consolidation" section (at plugins/dotnet-msbuild/skills/msbuild-modernization/SKILL.md:451) still shows <TargetFramework>net8.0</TargetFramework> inside a Directory.Build.props example — this directly contradicts the new AP-22 guidance being added here. Since the PR description mentions hardening existing guidance against misinterpretation, this source file should also be updated to remove TargetFramework from the Directory.Build.props example and add a comment explaining why it's excluded (similar to what was done in multi-level-examples.md).
Problem
Setting or in Directory.Build.props as a "default" is a common footgun. When .props sets the singular form and a project sets the plural form (or vice versa), both properties coexist and MSBuild simultaneously runs parts of "inner" and "outer" builds, and fails in otherwise difficult to understand ways.
Existing guidance (AP-21) covered conditioning on $(TargetFramework) in .props files, but nothing warned against setting the property itself. Worse, several places in the skills could be misread by agents as encouraging TFM centralization.
Changes
New antipattern entry (AP-22) in
additional-antipatterns.mdwith:DefaultTargetFrameworkworkaround for centralizing the version string safelyHardened existing guidance against misinterpretation:
directory-build-organizationSKILL.md: "Do NOT put here" now explicitly namesTargetFramework/TargetFrameworks(previously said "project-specific TFMs" which implied a shared TFM was OK)multi-level-examples.mdBefore/After: added note + XML comment explaining why TFM was deliberately not centralizedmsbuild-code-reviewagent andmsbuild-pr-reviewworkflow: added AP-22 as a detection check