mirror of
https://github.com/dotnet/skills.git
synced 2026-09-20 09:49:54 +08:00
Add TFM-aware buildTransitive->build forwarding guidance to msbuild skills (#836)
* Add TFM-aware buildTransitive->build forwarding guidance to msbuild skills buildTransitive/*.props should forward through the corresponding build/*.props (ownership chain buildTransitive -> build -> shared) rather than importing buildMultiTargeting/ directly. When build/ is packed per-TFM (build/<tfm>/), the forwarder must include the TFM segment and derive it from the file own folder, not $(TargetFramework) (NuGet nearest-match can serve a different asset folder), otherwise transitive consumers hit MSB4019. Updates extension-points (new Forwarding chain section), msbuild-antipatterns AP-13, and the msbuild-code-review agent. Lesson learned from microsoft/testfx#9431. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify ambiguous props/targets glob in forwarder review check Address review feedback on dotnet/skills#836: spell out ".props/.targets forwarders" instead of the ambiguous `buildTransitive/*.props|targets`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Tighten forwarding-chain guidance to reduce skill token overhead Condense the new extension-points Forwarding chain section (+26 -> +11 lines) and the AP-13 note by dropping the redundant non-TFM example and self-evident derivation explanation, keeping the chain rule, MSB4019 cause, and the TFM derivation expression. Lower token footprint addresses the skill-validator weighted-score token penalty without losing substance. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review nits: AP range and forward-slash path Update the additional anti-patterns range to AP-16 through AP-22 (the reference doc now includes AP-22), and use a forward-slash build/MyPackage.props in the forwarding-chain prose to match the build/<tfm>/ convention used in the section. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Condense AP-13 forwarding note to a cross-reference to reduce token overhead The full TFM-forwarding guidance lives in extension-points; AP-13 only needs a concise pointer. Removes duplication and trims the msbuild-antipatterns skill footprint flagged by the skill-validator token penalty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Address remaining review nits: targets forwarders, single-source wording, quoted MSBuildThisFileDirectory --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -52,6 +52,7 @@ Before starting any review, verify the context is MSBuild-related. If the worksp
|
||||
- Missing `PrivateAssets="all"` on analyzer packages?
|
||||
- Are there **property** conditions on `$(TargetFramework)` in `.props` files? (AP-21 — silently fails for single-targeting projects; move to `.targets`). See the AP-21 section in the [msbuild-antipatterns skill](../skills/msbuild-antipatterns/SKILL.md) for the full explanation. **Item and target conditions are NOT affected** and must not be flagged.
|
||||
- For any unguarded `<Import>` inside `build/<tfm>/` or `buildTransitive/<tfm>/`: resolve it against the **projected packed layout** recorded in Discovery. Do **not** flag as "missing Exists() guard" or "broken path" unless the target is missing from *both* the source tree and the packed layout. See `msbuild-antipatterns` AP-13 ("NuGet package forwarders" exception) and the `extension-points` skill ("Source Tree vs Packed Layout") for the rationale.
|
||||
- For `buildTransitive/` `.props`/`.targets` forwarders: prefer forwarding through the sibling `build/` file (ownership chain `buildTransitive → build → shared`) rather than importing `buildMultiTargeting/` directly. When `build/` is packed per-TFM (`build/<tfm>/`), the forwarder **must include the TFM segment** and derive it from the file's own folder (`$([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName('$(MSBuildThisFileDirectory)'))))`), **not** `$(TargetFramework)` — NuGet nearest-match can serve a different asset folder, and a missing segment fails with `MSB4019` for transitive consumers. See the `extension-points` skill ("Forwarding chain").
|
||||
- For backslash path separators (`\`) in `<Import Project=…>` or other path-typed evaluator inputs: do **not** report as a cross-platform 🔴 error. MSBuild normalizes them on Unix (`FileUtilities.MaybeAdjustFilePath`). Backslashes are only a real correctness defect in `<Exec Command=…>` raw shell strings, CDATA blocks, or paths handed verbatim to non-MSBuild consumers. See `msbuild-antipatterns` AP-14.
|
||||
|
||||
3. **Veracity gate** (run before producing the report): for each candidate 🔴 finding, ask:
|
||||
|
||||
@@ -104,6 +104,17 @@ MyPackage/
|
||||
- `build/` affects direct consumers only. `buildTransitive/` affects the entire dependency chain.
|
||||
- Props are imported early (before the project), targets are imported late (after the project).
|
||||
|
||||
### Forwarding chain: `buildTransitive/` → `build/` → shared
|
||||
|
||||
Forward `buildTransitive/*.props` and `buildTransitive/*.targets` through their sibling `build/*.props` / `build/*.targets` files (chain `buildTransitive → build → shared`) instead of importing `buildMultiTargeting/` directly. This keeps `build/` as the single source of truth with a clear ownership chain, so transitive consumers stay in sync with direct consumers instead of the two layouts drifting apart.
|
||||
|
||||
When `build/` is packed **per-TFM** (`build/<tfm>/`, via `TfmSpecificPackageFile`, a per-TFM `<PackagePath>`, or SDK conventions) while `buildMultiTargeting/` is not, a `buildTransitive/<tfm>/` forwarder **must include the TFM segment** — dropping it resolves to a non-existent package-root `build/MyPackage.props` and fails transitive consumers with **`MSB4019`**. Derive the segment from the file's own folder, never `$(TargetFramework)` (NuGet nearest-match can serve a `net10.0` consumer the `net9.0` folder, so `$(TargetFramework)` may name a folder that was never restored):
|
||||
|
||||
```xml
|
||||
<!-- buildTransitive/<tfm>/MyPackage.props -->
|
||||
<Import Project="$(MSBuildThisFileDirectory)..\..\build\$([System.IO.Path]::GetFileName($([System.IO.Path]::GetDirectoryName('$(MSBuildThisFileDirectory)'))))\MyPackage.props" />
|
||||
```
|
||||
|
||||
## Source Tree vs Packed Layout
|
||||
|
||||
When reviewing a NuGet build-extension package, the **source layout** in the repository can legitimately differ from the **packed layout** inside the produced `.nupkg`. This is a common source of false-positive "import points at a missing file" findings.
|
||||
|
||||
@@ -344,6 +344,8 @@ See `incremental-build` skill for deep guidance on Inputs/Outputs, FileWrites, a
|
||||
|
||||
Before flagging an unguarded `<Import>` inside a `build/` or `buildTransitive/` folder, **resolve it against the packed layout** — read every `*.nuspec` in the project directory **and its immediate parent directory** (shared nuspecs are common in mono-repos; do not walk further up), and any `<PackagePath>` metadata on `<None>`/`<Content>` items in the `.csproj`. Only flag if the target path is missing from **both** the source tree *and* the projected package layout. The `dotnet-msbuild/extension-points` skill — *Source tree vs packed layout* — documents the full cross-check procedure.
|
||||
|
||||
**Forwarding `buildTransitive/` → `build/`:** forward through the sibling `build/*.props` / `build/*.targets` file (not directly to `buildMultiTargeting/`); when `build/` is per-TFM (`build/<tfm>/`), include the TFM segment derived from the file's own folder (not `$(TargetFramework)`), or transitive consumers hit `MSB4019`. See the `extension-points` skill — *Forwarding chain* — for the rule and derivation expression.
|
||||
|
||||
---
|
||||
|
||||
## AP-14: Backslashes in Paths — Where It Matters
|
||||
|
||||
Reference in New Issue
Block a user