From 6af0e964ad0a74d1fc24c4a007db1b873ebbb7ae Mon Sep 17 00:00:00 2001 From: Kieron Lanning Date: Sun, 8 Nov 2026 18:09:34 +0000 Subject: [PATCH] perf: stop recompiling every project on every build; release 1.3.0 The SDK wrote GeneratedAssemblyInfo.cs and GeneratedAssemblyIdentifier.cs with Overwrite="true" and no WriteOnlyWhenDifferent. Both files are @(Compile) inputs, so CoreCompile could never be up to date and every build recompiled every project. Measured on a 39-project solution: a rebuild with no source changes ran 38 Csc invocations totalling 111.6 s of a 67.7 s build. It now runs none, and that rebuild takes 7.2 s instead of 67.7 s. Also in this release: - remove the AfterTargets="Build" skill recommendation hook, which read and regex-scanned every project assets file (about 500 KB, 256 libraries each) on every build to print advice nobody on CI could act on; the explicit VerifyPurviewSkills gate replaces it and escalates PRSKL0010/0011 to errors - read each .editorconfig in the style-policy chain once instead of twice - skip both repository-file bootstraps on design-time builds, which repeated the repository-root probes and two 95 KB reads per project per IDE edit - keep the tracked .editorconfig version marker in sync with package.json via just version-sync, checked by just version-sync-check in the pre-commit hook BREAKING CHANGE: no skill work runs during a build any more, and EnablePurviewSkillRecommendations, PurviewSkillRecommendationsInCI and PurviewSkillRecommendationStampPath are removed. Use -t:RecommendPurviewSkills to report, -t:VerifyPurviewSkills to fail a pipeline, and -t:InstallPurviewSkills to install. --- .config/lefthook.yml | 2 + .editorconfig | 2 +- AGENTS.md | 48 +++- CHANGELOG.md | 89 ++++++ Justfile | 11 +- README.md | 20 +- docs/wiki/Agent-Skills.md | 33 +-- docs/wiki/Getting-Started.md | 2 +- docs/wiki/Performance.md | 144 +++++++++- package.json | 2 +- scripts/sync-version.ts | 43 ++- .../references/properties.md | 3 +- .../SKILL.md | 35 ++- src/src/BuildSdk/Sdk/Props/Defaults.props | 21 +- src/src/BuildSdk/Sdk/Sdk.props | 9 - src/src/BuildSdk/Sdk/Sdk.targets | 271 +++++++++--------- .../BuildSdk.IntegrationTests.csproj | 1 + .../IncrementalBuildTests.cs | 96 +++++++ .../PackagedVersionFallbackTests.cs | 11 +- .../PurviewSkillRecommendationTests.cs | 147 +++------- .../RepositoryFileSyncTests.cs | 39 +++ templates/global.json | 2 +- 22 files changed, 681 insertions(+), 350 deletions(-) create mode 100644 src/tests/BuildSdk.IntegrationTests/IncrementalBuildTests.cs diff --git a/.config/lefthook.yml b/.config/lefthook.yml index cd41de9..cd475bf 100644 --- a/.config/lefthook.yml +++ b/.config/lefthook.yml @@ -2,6 +2,8 @@ pre-commit: jobs: - name: csharpier check run: just lint-check + - name: version pins + run: just version-sync-check commit-msg: jobs: diff --git a/.editorconfig b/.editorconfig index 848ca42..f8b3050 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,4 +1,4 @@ -# Purview.BuildSdk 1.0.7 - managed file, do not edit +# Purview.BuildSdk 1.3.0 - managed file, do not edit # # This file is written by Purview.BuildSdk and is replaced whenever the SDK version changes. The version # on the first line is how the SDK recognises a file it owns: a .editorconfig without that marker is diff --git a/AGENTS.md b/AGENTS.md index bacd71a..b1e7906 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -191,26 +191,43 @@ Two rules are worth knowing the reasoning for. A repository with **no** `skills/ Tests live in `src/tests/SkillsTesting.UnitTests/`, with the repository-level ones in `src/tests/BuildSdk.IntegrationTests/RepositorySkillTests.cs`, `SkillRepositoryRootParityTests.cs` and `RepositorySkillsSelfTests.cs`. -### Recommending skills +### Checking skills -Installation is explicit, but **recommendation runs during an ordinary build** — it is the one skill feature that does. `RecommendPurviewSkillsOnBuild` (`AfterTargets="Build"`) delegates to `RecommendPurviewSkills`, which can also be invoked by name: +**Nothing about skills runs during a build.** This used to be the opposite — a +`RecommendPurviewSkillsOnBuild` hook ran on `AfterTargets="Build"` — and it was removed after +measurement: on a 39-project solution it read and regex-scanned every project's +`project.assets.json` (≈500 KB, 256 libraries each), made ~10 000 failing `Directory.Exists` calls, +read the lockfile and walked the repository root, on every build, to print advice that a repository +managing its own skills had already acted on. Every input is repository-scoped, so the per-project +cost was pure duplication. The remaining work is now paid only when somebody asks: ```bash -dotnet msbuild -t:RecommendPurviewSkills +dotnet msbuild -t:RecommendPurviewSkills # report, never fail +dotnet msbuild -t:VerifyPurviewSkills # report, and fail on anything unverifiable +dotnet msbuild -t:InstallPurviewSkills # install what is missing +dotnet msbuild -t:ListPurviewSkills # what the restored packages ship ``` +`VerifyPurviewSkills` is the gate a CI job calls. It escalates `PRSKL0010` (missing) and `PRSKL0011` +(drifted) from warning to **error** — both are factual, both are fixed by one command, and neither can +fire without a referenced package that really ships skills. `PRSKL0012` stays a warning, because a +package published without repository metadata is not the consumer's to fix; `PRSKL0013`/`PRSKL0014` +stay messages, because 0013 is an owner-scoped heuristic and 0014 fires on any clone of a repository +that installs skills on demand. Failing a build over either would hand people errors they cannot +clear, which is how a gate gets switched off. The escalation is safe because the target cannot be +reached by accident: a build that did not ask for it is never broken by skill advice. + `EvaluatePurviewSkillState` (inline task in `Sdk/Sdk.targets`) joins the skills each restored package ships against the CLI's own `skills-lock.json` and classifies every one: satisfied, missing (`PRSKL0010`), ref-drifted (`PRSKL0011`), unverifiable (`PRSKL0012`), orphaned (`PRSKL0013`), or recorded-but-never-restored (`PRSKL0014`). Both inputs are local, so it stays offline and spawns no process. The lockfile is read directly rather than by parsing `npx skills list --json`, because that listing omits the `ref` — and the ref is the whole compatibility question. Reading a file also needs no Node tooling, which is what keeps recommendations working on a machine that can build .NET but has no Node. There is deliberately no competing lockfile. Drift **never** recommends `npx skills update`: `update` installs the newest published skill, which is only correct when the newest skill happens to describe the referenced package version. `PRSKL0011` names the referenced package's own commit in a pinned `npx skills add` instead. -Four things keep it quiet, and all of them are load-bearing: +Three things shape the output, and all of them are load-bearing: -- design-time builds (`DesignTimeBuild`), CI builds (`ContinuousIntegrationBuild`/`TF_BUILD`/`GITHUB_ACTIONS`/`CI` unless `PurviewSkillRecommendationsInCI=true`), non-default TFM passes and `EnablePurviewSkillRecommendations=false` are all skipped outright; -- a fingerprint stamp (`/.purview/skill-recommendations.cache`) means a given state is reported once per repository rather than once per project and again on every rebuild — the explicit target ignores the stamp; -- missing and drifted are warnings (actionable in one command), while unverifiable and orphaned are messages, so no amount of skill advice can break a build using `TreatWarningsAsErrors`; and +- missing and drifted are warnings (actionable in one command), while unverifiable and orphaned are messages — and `VerifyPurviewSkills` is what turns them into failures; and - a correctly installed set produces no output at all. +- there is no deduplication stamp any more (`EnablePurviewSkillRecommendations`, `PurviewSkillRecommendationsInCI` and `PurviewSkillRecommendationStampPath` are retired with it). A stamp existed so a per-build hook could report one state once per repository; a target a person invokes should answer every time. Orphan detection is **owner-scoped on purpose**: a lockfile cannot record who asked for an entry, so only an entry under an owner the project references — but from a repository it no longer references — is reported. A skill from any other owner is the developer's own and is never mentioned. @@ -279,13 +296,16 @@ Prefer `just` tasks: - `just pack` - `just skills-validate` — the pack-time check over the authored skills - `just skills-add ` — install a skill from another repository into `.agents/skills/` -- `just version-sync` — rewrite the `Purview.BuildSdk` pins in the docs and `templates/global.json` - after bumping `package.json` - -Nothing packed carries a hand-maintained version. The `.editorconfig` marker and -`Props/PackedVersion.props` (the `global.json` fallback when the SDK is not in a versioned package -folder) are stamped at pack time from `$(PackageVersion)`. Only the copy-paste pins in the docs live in -git, and `PackagedVersionFallbackTests` names each stale one as `file:line`. +- `just version-sync` — rewrite the `Purview.BuildSdk` pins in the docs, `templates/global.json` and + `.editorconfig` marker after bumping `package.json` +- `just version-sync-check` — the same check without writing; what the pre-commit hook runs + +Nothing packed carries a hand-maintained version. `Props/PackedVersion.props` (the `global.json` +fallback when the SDK is not in a versioned package folder) is stamped at pack time from +`$(PackageVersion)`. Only the copy-paste pins live in git — the quick-start snippets, the starter +template, and this repository's own `.editorconfig` marker, so that reading the tracked file never +shows a release-old number. `PackagedVersionFallbackTests` names each stale one as `file:line`, and +`just version-sync-check` fails the commit before it can land. `dotnet` fallback uses `src/BuildSdk.slnx` and `Release`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f9256e..4d1289f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,95 @@ release summarise what the commits recorded rather than what was written at the ## [Unreleased] +## [1.3.0] - 2026-11-08 + +### Changed (breaking) + +- **The skill recommendation hook is gone. No skill work happens during a build.** `RecommendPurviewSkillsOnBuild` + (`AfterTargets="Build"`) and its `_PurviewSkillRecommendationBuildSetup` helper are removed. On a + 39-project solution the hook read and regex-scanned every project's `project.assets.json` + (≈500 KB, 256 libraries each), made thousands of failing `Directory.Exists` calls, read the lockfile + and walked the repository root — on every build, including a no-change rebuild — and every one of its + inputs is repository-scoped, so the per-project cost was pure duplication of one repository-wide + answer. Removed rather than defaulted off, because a repository that manages its skills deliberately + (any repository with a `skills-lock.json`) was paying twice for an answer it had already acted on. + Use `-t:RecommendPurviewSkills` to report and `-t:InstallPurviewSkills` to install. + +- **Removed properties:** `EnablePurviewSkillRecommendations`, `PurviewSkillRecommendationsInCI` and + `PurviewSkillRecommendationStampPath`. All three existed to gate and deduplicate the hook, including + the `/.purview/skill-recommendations.cache` stamp. A target a person invokes should answer + every time it is asked, so no stamp is needed. They are also no longer compiler-visible. + +### Added + +- **`VerifyPurviewSkills`** — the skill state as a gate rather than advice. It reports the same + classifications as `RecommendPurviewSkills` but escalates `PRSKL0010` (missing) and `PRSKL0011` + (drifted) to **errors**, which is what a CI job should call. Those two are always factual and always + fixed by one `npx skills add` command. `PRSKL0012` is reported as a warning rather than an error, + because it also covers a package published without repository metadata — something only that + package's author can fix — and `PRSKL0013`/`PRSKL0014` stay messages, because 0013 is an owner-scoped + heuristic and 0014 fires on any clone of a repository that installs skills on demand. Safe because the + target is only reachable by name: a build that did not ask is never broken by skill advice. + +```bash +dotnet msbuild -t:VerifyPurviewSkills +``` + +### Fixed + +- **Incremental builds were broken outright.** `GenerateAssemblyInfoClassTarget` and + `GenerateAssemblyIdentifierClassTarget` wrote their generated files with `Overwrite="true"` and no + `WriteOnlyWhenDifferent`, giving them a fresh timestamp on every build. Both files are compile inputs, + so `CoreCompile` — whose `Inputs` are `@(Compile)` — could never be up to date, and a no-change rebuild + recompiled every project. Measured on a 39-project solution: a warm rebuild ran **38** `Csc` + invocations totalling **111 s** of a 68 s build. `WriteOnlyWhenDifferent="true"` makes an unchanged + build compile nothing. This also restores the value of `AccelerateBuildsInVisualStudio=true`, which + defaults to on and was being defeated by the same timestamps. + +- **`ValidatePurviewStylePolicy` read every `.editorconfig` in the chain twice** — once to find + `root = true` and once to merge it. The SDK's own `.editorconfig` is 2,714 lines / 95 KB, so each + project read it twice on every compile. The file is now read once and the lines are shared. + +- **Both repository-file bootstraps ran during design-time builds.** `EnsureRepositoryEditorConfigTarget` + and `EnsureRepositoryGlobalJsonTarget` were not guarded against `DesignTimeBuild`, and an IDE runs a + design-time build for every edit, on every project, for as long as a solution is open. Each pass + repeated the repository-root probe ladder and two full reads of the 95 KB `.editorconfig` + (0.83 ms of I/O per project, per pass) and could never produce anything a design-time build uses. + Both are now skipped when `DesignTimeBuild=true`; a real build still bootstraps them, which is the + only thing that ever needed to. + +- **`BuildSdk.IntegrationTests` had the same unconditional-rewrite defect**, in its own + `WriteSdkSourcePaths` target: `Generated/SdkPaths.cs` was written with `Overwrite="true"` and no + `WriteOnlyWhenDifferent`, so that project recompiled on every build even after the two SDK writers + were fixed. It is fixed the same way, and a `Release` build of this repository with no changes now + runs **zero** `Csc` invocations in 0.85 s. + +### Tests + +- `IncrementalBuildTests` pins both SDK-generated files: building an unchanged project twice must + leave `GeneratedAssemblyInfo.cs` and `GeneratedAssemblyIdentifier.cs` with their original + timestamps and content. A rewrite is what breaks incrementality, and a rewrite always moves the + timestamp, so the test asserts the timestamp rather than counting compiler invocations. + +### Repository + +- **The tracked `.editorconfig` marker now self-syncs.** It names the SDK version that wrote the file, + and it had drifted to `1.0.7` while `package.json` was on `1.2.x` — misleading for anyone reading the + repository, though never wrong for a consumer, because packing stamps the version in. The file is now + in `scripts/sync-version.ts`, so `just version-sync` rewrites it along with the quick-start snippets, + `AdoptionPin_MatchesPackageJson` asserts it, and a new `just version-sync-check` fails the pre-commit + hook when it is stale. `CHANGELOG.md` is deliberately excluded: its marker lines are history and keep + the version they shipped with. + +### Notes + +- `docs/wiki/Performance.md` now carries measured numbers for a 39-project solution instead of a list + of things that had not been measured. The headline, measured end to end on `changeops` with this + version: a rebuild with **no source changes went from 67.7 s to 7.2 s**, and from **38 `Csc` + invocations to 0**. A cold build went from 154.4 s to 103.2 s. Note that the *first* warm build after + upgrading is slower than that (27.3 s), because the upgrade invalidates analyzers, `.editorconfig` + and compiler state at once; steady state is reached on the next no-change build. + ## [1.2.0] - 2026-10-28 ### Added diff --git a/Justfile b/Justfile index a737d6a..e0f62fc 100644 --- a/Justfile +++ b/Justfile @@ -88,7 +88,7 @@ restore *args: current_version: echo "==> Current version: {{ GREEN }}{{ current_version }}{{ NORMAL }} (defined in package.json and automatically included in the build output through the Purview.BuildSdk package)" -# Rewrite the Purview.BuildSdk pins in the docs and starter template to the version in package.json +# Rewrite the Purview.BuildSdk pins in the docs, starter template and .editorconfig marker # # Run after bumping package.json. Packed files are stamped at pack time and need nothing. [group('Build and Test')] @@ -96,6 +96,15 @@ version-sync: echo "==> Syncing version pins to {{ GREEN }}{{ current_version }}{{ NORMAL }}" bun scripts/sync-version.ts +# Check the version pins without writing anything; non-zero when a pin is stale +# +# What the pre-commit hook runs, so a stale .editorconfig marker or quick-start snippet cannot be +# committed in the first place. +[group('Build and Test')] +version-sync-check: + echo "==> Checking version pins against {{ GREEN }}{{ current_version }}{{ NORMAL }}" + bun scripts/sync-version.ts --check + # Run tests for a specific project with a filter (e.g., "/*/*/*/*/", or "/*/*/*/*[Category=Unit]" to run just unit tests) and configuration (e.g., "Release") [group('Build and Test')] test filter="/*/*/*/*/" *args: diff --git a/README.md b/README.md index b09af0b..79012d0 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ A reusable MSBuild SDK NuGet package that delivers standardised .NET project def "runner": "Microsoft.Testing.Platform" }, "msbuild-sdks": { - "Purview.BuildSdk": "1.2.0" + "Purview.BuildSdk": "1.3.0" } } ``` @@ -201,7 +201,7 @@ to a low-importance message) and only a copy that still fails after every retry The bootstrapped `.editorconfig` opens with a version marker: ```editorconfig -# Purview.BuildSdk 1.2.0 - managed file, do not edit +# Purview.BuildSdk 1.3.0 - managed file, do not edit ``` That line is how the SDK recognises a file **it** wrote. When the SDK version changes, a file carrying @@ -290,9 +290,13 @@ dotnet msbuild -t:InstallPurviewSkills # explicit; delegates to the Compatibility is pinned by commit: a package's nuspec names the commit it was built from, the CLI accepts `owner/repo#` and records it, so a project on an older package version is never -advised to install skills describing a newer API. Set `EnablePurviewSkillRecommendations=false` to -turn the build-time reporting off; it is already silent on CI, during design-time builds, and when -everything is installed and compatible. +advised to install skills describing a newer API. + +**Nothing about skills runs during a build.** `-t:RecommendPurviewSkills` reports the state and +`-t:VerifyPurviewSkills` reports it and fails when a referenced package's skills are missing or pinned +to a different ref — the second is what a CI job calls. Reading the restore graph and the lockfile on +every build of every project was measured as the largest avoidable cost in an up-to-date build, so it +no longer happens unless somebody asks for it. The CLI is launched through a package runner. `PurviewSkillsRunner` defaults to `npx` — which ships with Node itself — and also accepts `bunx`, `bun`, `pnpm`, `yarn`, `deno` or `skills`; anything else @@ -416,9 +420,6 @@ Non-packable projects (including web applications) default `WarnOnPackingNonPack | Property | Default | Description | | -- | -- | -- | -| `EnablePurviewSkillRecommendations` | `true` | Reports missing or version-drifted agent skills after an ordinary build. Offline; never installs; never fails the build. | -| `PurviewSkillRecommendationsInCI` | `false` | When `true`, recommendations are reported on CI builds as well. | -| `PurviewSkillRecommendationStampPath` | `/.purview/skill-recommendations.cache` | Overrides the stamp that reports each distinct skill state once per repository. | | `PurviewSkillsLockFile` | `/skills-lock.json` | Overrides where the skills CLI's lockfile is read from. | | `PurviewSkillsSourceFolder` | `/skills` | Repository folder holding authored skills. | | `PurviewSkillsAgents` | `*` | Agents that `InstallPurviewSkills` installs to, and that recommended commands name. | @@ -521,9 +522,6 @@ The SDK now exports its properties via `CompilerVisibleProperty`, so analyzers a | `PurviewRepoBootstrapCopyFailureAsError` | When `true` (default), a bootstrap write that still fails after every retry is an error. | | `PurviewSuppressCopyRetryWarnings` | When `true` (default), built-in copy retry notices (`MSB3026`) are demoted to messages. | | `PurviewAutoSdkPack` | When `true`, automatically packs the `Sdk/` folder contents into the NuGet package with the correct root-level paths. | -| `EnablePurviewSkillRecommendations` | When `true` (default), an ordinary build reports missing or version-drifted agent skills. | -| `PurviewSkillRecommendationsInCI` | When `true`, skill recommendations are reported on CI builds as well (default `false`). | -| `PurviewSkillRecommendationStampPath` | Stamp used to report each distinct skill state once (default `/.purview/skill-recommendations.cache`). | | `PurviewSkillsLockFile` | Where the skills CLI's `skills-lock.json` is read from (default `/skills-lock.json`). | | `PurviewSkillsSourceFolder` | Repository folder holding authored agent skills (default `/skills`). | | `PurviewSkillsAgents` | Agents that `InstallPurviewSkills` installs to (default `*`). | diff --git a/docs/wiki/Agent-Skills.md b/docs/wiki/Agent-Skills.md index 4817da9..fd0918a 100644 --- a/docs/wiki/Agent-Skills.md +++ b/docs/wiki/Agent-Skills.md @@ -156,29 +156,26 @@ repository it *no longer* references, is reported. A skill from any other owner own and is never mentioned. This is a heuristic, which is why it is a message and why nothing is ever removed automatically. -### Staying quiet +### When it runs -| Situation | Behaviour | +**Never during a build.** There is no `AfterTargets="Build"` hook: reading the restore graph and +the lockfile for every project on every build was measured as the largest avoidable cost in an +otherwise up-to-date build, and its only output was advice. Each target below is reached only +because somebody named it. + +| Target | Behaviour | | -- | -- | -| Everything installed and compatible | No output at all | -| Design-time build (`DesignTimeBuild=true`) | Skipped | -| CI build (`ContinuousIntegrationBuild`, `TF_BUILD`, `GITHUB_ACTIONS`, `CI`) | Skipped unless opted in | -| Non-default target framework in a multi-targeting build | Skipped | -| Same state as last reported | Skipped | - -The last one is deduplication. A solution has many projects sharing one repository, and a rebuild -changes nothing, so a fingerprint of the reported state is kept in -`/.purview/skill-recommendations.cache`. The same advice is printed once, and again only -when the state it describes actually changes. The explicit `-t:RecommendPurviewSkills` target -ignores the stamp and always answers. +| `RecommendPurviewSkills` | Reports the state. Never fails a build. | +| `VerifyPurviewSkills` | The same report, escalating `PRSKL0010`/`PRSKL0011` to an error. The CI gate. | +| `InstallPurviewSkills` | Installs what is missing, via the skills CLI. | +| `ListPurviewSkills` | Lists what the restored packages ship. Installs nothing. | + +A correctly installed set produces no output at all from either reporting target. ### Configuration | Property | Default | Description | | -- | -- | -- | -| `EnablePurviewSkillRecommendations` | `true` | Reports skill state after an ordinary build. | -| `PurviewSkillRecommendationsInCI` | `false` | Set `true` to report on CI builds too. | -| `PurviewSkillRecommendationStampPath` | `/.purview/skill-recommendations.cache` | Overrides the deduplication stamp. | | `PurviewSkillsLockFile` | `/skills-lock.json` | Overrides where the CLI's lockfile is read from. | | `PurviewSkillsAgents` | `*` | Agents named in the recommended install command. | | `PurviewSkillsRunner` | `npx` | Package runner used to launch the CLI. | @@ -187,6 +184,10 @@ ignores the stamp and always answers. | `PurviewSkillsInstallRoot` | *(resolved)* | Overrides the repository root every skill target runs from. | | `PurviewRepositoryRootMarkers` | `.git/;.github/;.gitattributes;LICENSE.md;AGENTS.md;.agents/;CLAUDE.md;.claude/;package.json` | Ordered markers that identify a repository root; the first is definitive. | +Removed in this release, with no replacement needed: `EnablePurviewSkillRecommendations`, +`PurviewSkillRecommendationsInCI` and `PurviewSkillRecommendationStampPath`. They existed to gate and +deduplicate a build hook that no longer exists. + ## Version compatibility NuGet package versions and skill versions evolve independently, so a project on an older library diff --git a/docs/wiki/Getting-Started.md b/docs/wiki/Getting-Started.md index 1e20ee4..fc5f045 100644 --- a/docs/wiki/Getting-Started.md +++ b/docs/wiki/Getting-Started.md @@ -14,7 +14,7 @@ Add `Purview.BuildSdk` to the `msbuild-sdks` section so MSBuild can resolve the "runner": "Microsoft.Testing.Platform" }, "msbuild-sdks": { - "Purview.BuildSdk": "1.2.0" + "Purview.BuildSdk": "1.3.0" } } ``` diff --git a/docs/wiki/Performance.md b/docs/wiki/Performance.md index 57fe7ca..5092ad2 100644 --- a/docs/wiki/Performance.md +++ b/docs/wiki/Performance.md @@ -64,14 +64,142 @@ The regression net is `BuildSdk.IntegrationTests`, which asserts evaluated prope through `ProjectHarness.GetPropertyAsync` against real `dotnet msbuild` invocations. All 406 tests pass unchanged. -## Inline tasks +## Measured: a real 39-project solution + +Measured on `changeops` (`src/ChangeOps.slnx`, 39 projects, `net11.0`, `AnalysisMode=All`, +`TreatPurviewWarningsAsErrors`-style strictness, consuming `Purview.BuildSdk` from `global.json` +`msbuild-sdks`), with `dotnet build -clp:PerformanceSummary`. That flag prints per-target and +per-task cumulative time, so none of this needs a binlog viewer. + +### The headline: incremental builds did not work + +| | Cold build | Warm rebuild, no source changes | +| -- | -- | -- | +| Wall clock | 154.4 s | **67.7 s** | +| `Csc` invocations | 22 | **38** | +| `Csc` time | 97.9 s | **111.6 s** | + +Every one of the 38 projects recompiled on a build where nothing had changed. The cause was +`GenerateAssemblyInfoClassTarget` and `GenerateAssemblyIdentifierClassTarget` writing their generated +files with `Overwrite="true"` and no `WriteOnlyWhenDifferent`: both files are `@(Compile)` inputs, so +`CoreCompile` never had an up-to-date output. That single attribute was worth more than everything +else in this document combined, and it also silently defeated the `AccelerateBuildsInVisualStudio=true` +default that exists to make up-to-date checks fast. + +### Where the rest of the time went + +Task-level numbers from the cold build (`Target Performance Summary` is unreliable on a solution this +wide, because `Build` and `ResolveProjectReferences` nest and each project instance reports its own +copy of the same wait): + +| Task | Time | Calls | +| -- | -- | -- | +| `Csc` | 97,857 ms | 22 | +| `Microsoft.Build.Tasks.Git.LocateRepository` + `GetUntrackedFiles` (SourceLink) | 4,095 ms | 76 | +| `ResolvePurviewSkillSources` | 1,473 ms | 38 | +| `EvaluatePurviewSkillState` | 357 ms | 38 | +| `FindPurviewRepositoryRoot` | 270 ms | 38 | +| `ValidatePurviewStylePolicy` | 195 ms | 38 | +| `ReadBuildTransitiveCompilerVisibleProperties` | 98 ms | 36 | + +`ResolvePurviewSkillSources` alone was 1.47 s, and it could only ever answer "no restored package +ships agent skills" for that repository — the restored `Purview.BuildSdk` package carries no +`.agents/` folder. It read and regex-scanned 38 separate ≈500 KB `project.assets.json` files to reach +an answer that is identical for all 38 projects. + +### What changed as a result + +| Change | Effect | +| -- | -- | +| `WriteOnlyWhenDifferent="true"` on both generated-file writers | A no-change rebuild compiles nothing instead of everything | +| The `AfterTargets="Build"` skill hook removed; `VerifyPurviewSkills` added | 2.1 s and three inline-task compilations removed from every build | +| `ValidatePurviewStylePolicy` reads each `.editorconfig` once | 95 KB × 2 → 95 KB per project per compile | +| Both repo-file bootstraps skip `DesignTimeBuild` | No probe ladder and no 2 × 95 KB read per project, per IDE edit | + +### After: the same solution on 1.3.0 + +The 1.2.0 figures above are the "before". Re-measured on `changeops` after the fixes, same machine, +same solution, no source changes between runs: + +| | 1.2.0 | 1.3.0 | +| -- | -- | -- | +| Cold build | 154.4 s | 103.2 s | +| Warm rebuild, no source changes | 67.7 s | **7.20 s** | +| `Csc` invocations on the warm rebuild | **38** | **0** | +| `Csc` time on the warm rebuild | 111.6 s | 0 | -`Sdk.targets` defines 10 `RoslynCodeTaskFactory` inline C# tasks. MSBuild compiles each on first use, -per node, so they carry a cold-start cost that is paid once per build rather than once per project. -This has not been measured, and is the obvious next thing to look at. +Two details worth having on record: -## Not yet measured +- **The first warm build after upgrading to 1.3.0 took 27.3 s, not 7.2 s**, because the SDK upgrade + changes enough (analyzers, `.editorconfig`, editorconfig content) to invalidate a lot at once. Only + the second no-change build reaches steady state. Quoting the 27.3 s figure as the steady state would + overstate what is left by a factor of four. +- Every Purview target in the steady-state build sums to roughly **0.9 s of the 7.2 s** — about 12%, + with no single one above 400 ms (`PurviewValidateBrowserSafeReferences` 362 ms, + `EnsureRepositoryEditorConfigTarget` 268 ms, `ValidatePurviewStylePolicy` 62 ms). The SDK is no + longer the story; framework and ecosystem targets are. -- End-to-end evaluation of a synthetic 50- and 200-project solution, with and without the SDK imported. -- Inline-task compilation cost, cold versus warm. -- `VersionDetection` cache hit rate across a solution. +### `.editorconfig` work, sized honestly + +Measured rather than guessed, because the numbers change the decision: + +| Work | Cost | +| -- | -- | +| Two full reads of the 95 KB `.editorconfig` + two first-line regexes | **0.83 ms per project**, ≈ 32 ms per 39-project build | +| `ValidatePurviewStylePolicy` task (after reading each file once) | 170 ms per build, 38 projects | +| `EnsureRepositoryEditorConfigTarget` target | 381 ms per build, 38 projects | + +So the redundant read is small in absolute terms. What made it worth changing was not the size but +*when* it ran: neither bootstrap target was guarded against design-time builds, and an IDE runs one +of those for every edit, on every project, for the whole time a solution is open. The fix is a +`DesignTimeBuild` condition, not a cleverer read. + +The second read is **kept deliberately**. It looks removable — the marker line is stamped as +`markerPrefix + version + " - managed file, do not edit"`, so the SDK could compute the line it +expects instead of reading the source. It cannot: in an unpacked source tree no version is +available (`Props/PackedVersion.props` only exists in a packed SDK, and the package folder is not +versioned), so the computed line would never match, `_PurviewEditorConfigNeedsSync` would be true on +every build, and `SyncPurviewRepositoryFiles` — an inline task with a measured ~680 ms first-use +compile — would be invoked every time. Reading the source is what makes the check robust; 0.83 ms is +a fair price for not having a second place that knows the marker format. + +### Verified on this repository + +This repository dogfoods the SDK from source (`src/Directory.Build.props`), so a build of +`src/BuildSdk.slnx` exercises the changes directly. Three consecutive `Release` builds, each after +the previous one had absorbed an edit: + +| Build | Wall clock | `Csc` invocations | +| -- | -- | -- | +| Absorbs edits | 10.8 s | 6 | +| No changes | 6.4 s | 1 | +| No changes, after the last generated-file writer was fixed | **0.85 s** | **0** | + +The intermediate 6.4 s / 1-compile case is worth keeping: it was +`src/tests/BuildSdk.IntegrationTests/BuildSdk.IntegrationTests.csproj` doing the same thing by hand +— its `WriteSdkSourcePaths` target wrote `Generated/SdkPaths.cs` with `Overwrite="true"` and no +`WriteOnlyWhenDifferent`. The same defect, independently authored, in a file the SDK does not own. +`IncrementalBuildTests` now pins both SDK-generated files so neither can regress. + +## Deliberately not changed + +Two findings were measured, judged, and left alone. Both are recorded so they are not re-litigated: + +- **`ResolvePurviewSkillSources` still regex-scans the whole assets file.** It is now off the build + path entirely, so the cost is paid only by an explicitly invoked target: 1.47 s across 39 projects. + A substring pre-check (`.agents/skills`) would remove almost all of it, but doing so means + re-indenting ~90 lines of the SDK's largest file for no benefit to an ordinary build. +- **`ImportProjectReferencedBuildTransitiveAssets` is not worth changing either**, and this is worth + recording because it looked like the obvious next target. Its target-level timing (≈2 s across 38 + projects) is inflated by nesting, and on a transitional build it appeared to cost 19 s. Toggling it + off on the steady-state build gave 7.99 s against 7.20 s with it on — i.e. nothing. The recursive + `Directory.GetFiles` is not a real cost, so the safer `TopDirectoryOnly`/memoisation work has no + measured justification and was left undone. + +The largest remaining per-target cost in that solution is not this SDK at all: +`MsCoverageExtensionReferencedPathMaps`, 13,187 ms over 16 calls (≈824 ms per test project) on a build +that compiled nothing. It comes from the Microsoft coverage extension, and it is the next thing worth +investigating in that repository. + +`AnalysisMode=All` (`Props/Defaults.props`) is deliberate policy rather than an accident, so its +default does not change. `-p:ReportAnalyzer=true` measures what it costs in a given repository. diff --git a/package.json b/package.json index 7dc5c6d..f496cfd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "build-sdk", - "version": "1.2.0", + "version": "1.3.0", "homepage": "https://purview.dev/projects/build-sdk/", "bugs": { "url": "https://github.com/purview-dev/build-sdk/issues" diff --git a/scripts/sync-version.ts b/scripts/sync-version.ts index c7548d5..b27aab0 100644 --- a/scripts/sync-version.ts +++ b/scripts/sync-version.ts @@ -1,40 +1,65 @@ -// Rewrites the Purview.BuildSdk version pins that an adopter copies by hand - the quick-start snippets -// and the starter template - to the version in package.json, the single source of truth. +// Rewrites the Purview.BuildSdk version pins that an adopter copies by hand - the quick-start snippets, +// the starter template, and this repository's own .editorconfig marker - to the version in package.json, +// the single source of truth. // // Only text that is copied verbatim lives here. Everything that is packed is stamped at pack time // instead (see StampPurviewPackedVersionTarget and StampPurviewEditorConfigVersionTarget in // BuildSdk.csproj), so it never needs syncing. PackagedVersionFallbackTests fails the build if a file // listed here drifts, and names this script as the fix. // -// just version-sync +// just version-sync rewrite every stale pin +// just version-sync-check report drift and exit non-zero, writing nothing +// +// CHANGELOG.md is deliberately absent. Its marker lines are history and have to keep the version they +// shipped with; only the first line of .editorconfig is live, and it carries the marker exactly once. import { readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; const root = join(import.meta.dir, ".."); const { version } = JSON.parse(readFileSync(join(root, "package.json"), "utf8")); +const checkOnly = process.argv.includes("--check"); -const files = ["README.md", "docs/wiki/Getting-Started.md", "templates/global.json"]; +const files = ["README.md", "docs/wiki/Getting-Started.md", "templates/global.json", ".editorconfig"]; const replacements: [RegExp, string][] = [ // The global.json pin. A '$(...)' property reference documents auto-detection and is not a pin. [/("Purview\.BuildSdk"\s*:\s*")(?!\$)[^"]+(")/g, `$1${version}$2`], - // The example marker line of a bootstrapped .editorconfig. + // The example marker line of a bootstrapped .editorconfig, and the live one in this repository's own. [/(# Purview\.BuildSdk )\S+( - managed file)/g, `$1${version}$2`], ]; let changed = 0; +const stale: string[] = []; for (const file of files) { const path = join(root, file); const before = readFileSync(path, "utf8"); const after = replacements.reduce((text, [pattern, replacement]) => text.replace(pattern, replacement), before); - if (after !== before) { - writeFileSync(path, after); - changed++; - console.log(` updated ${file}`); + if (after === before) { + continue; + } + + if (checkOnly) { + stale.push(file); + continue; + } + + writeFileSync(path, after); + changed++; + console.log(` updated ${file}`); +} + +if (checkOnly) { + if (stale.length > 0) { + console.error(` stale at ${version}: ${stale.join(", ")}`); + console.error("Run `just version-sync` to update them."); + process.exit(1); } + + console.log(`All pins already at ${version}.`); + process.exit(0); } console.log(changed === 0 ? `All pins already at ${version}.` : `Pinned ${changed} file(s) to ${version}.`); diff --git a/skills/purview-buildsdk-configuration/references/properties.md b/skills/purview-buildsdk-configuration/references/properties.md index 1fb1f81..3b22bc8 100644 --- a/skills/purview-buildsdk-configuration/references/properties.md +++ b/skills/purview-buildsdk-configuration/references/properties.md @@ -135,8 +135,7 @@ nothing else was left for the mirror to deliver. Covered in full by `purview-buildsdk-skill-installation`: `PurviewSkillsRunner`, `PurviewSkillsRunnerCandidates`, `PurviewSkillsCommand`, `PurviewSkillsPackage`, -`PurviewSkillsAgents`, `PurviewSkillsSource`, `PurviewSkillsInstallRoot`, `PurviewSkillsRepoRoot`, -`EnablePurviewSkillRecommendations`, `PurviewSkillRecommendationsInCI`. +`PurviewSkillsAgents`, `PurviewSkillsSource`, `PurviewSkillsInstallRoot`, `PurviewSkillsRepoRoot`. ## Compiler-visible properties diff --git a/skills/purview-buildsdk-skill-installation/SKILL.md b/skills/purview-buildsdk-skill-installation/SKILL.md index 66c20a3..59bfb54 100644 --- a/skills/purview-buildsdk-skill-installation/SKILL.md +++ b/skills/purview-buildsdk-skill-installation/SKILL.md @@ -74,29 +74,40 @@ The caller owns the ref here — append `#` to pin it. This path needs no r | `PRSKL0013` | An orphaned lockfile entry | Message | | `PRSKL0014` | Recorded in the lockfile but never restored | Message | -## Recommendations run during an ordinary build +## Nothing about skills runs during a build -This is the **one** skill feature that does. `RecommendPurviewSkillsOnBuild` runs after `Build` and -delegates to `RecommendPurviewSkills`, which can also be invoked by name: +This used to be the opposite: a `RecommendPurviewSkillsOnBuild` hook ran on `AfterTargets="Build"`. +It was removed after measurement — on a 39-project solution it read and regex-scanned every project's +`project.assets.json` (≈500 KB, 256 libraries each), made thousands of failing `Directory.Exists` +calls, read the lockfile and walked the repository root, on every build, to print advice that a +repository managing its own skills had already acted on. All of its inputs are repository-scoped, so +the per-project cost bought nothing. + +The work is now paid only when somebody names a target: ```bash -dotnet msbuild -t:RecommendPurviewSkills +dotnet msbuild -t:RecommendPurviewSkills # report, never fail +dotnet msbuild -t:VerifyPurviewSkills # report, and fail on PRSKL0010/0011 ``` `EvaluatePurviewSkillState` joins the skills each restored package ships against the CLI's own `skills-lock.json`. Both inputs are local files, so it stays offline and spawns no process. -Four things keep it quiet, and all of them are load-bearing: +`VerifyPurviewSkills` is the gate to call from CI. It escalates only `PRSKL0010` (missing) and +`PRSKL0011` (drifted) to errors: both are factual, both are fixed by one command, and neither can fire +without a referenced package that really ships skills. `PRSKL0012` stays a warning because a package +published without repository metadata is not the consumer's to fix, and `PRSKL0013`/`PRSKL0014` stay +messages. Failing over something the caller cannot clear is how a gate gets switched off. + +Three things shape the output: -- Design-time builds, CI builds, non-default TFM passes and - `EnablePurviewSkillRecommendations=false` are skipped outright. - `PurviewSkillRecommendationsInCI=true` re-enables CI. -- A fingerprint stamp at `/.purview/skill-recommendations.cache` means a given state is - reported once per repository, not once per project and again on every rebuild. The explicit target - ignores the stamp. - Missing and drifted are **warnings** (fixable in one command); unverifiable and orphaned are - **messages**. No amount of skill advice can break a build using `TreatWarningsAsErrors`. + **messages**. `VerifyPurviewSkills` is what turns them into failures. - A correctly installed set produces no output at all. +- There is no deduplication stamp any more. `EnablePurviewSkillRecommendations`, + `PurviewSkillRecommendationsInCI` and `PurviewSkillRecommendationStampPath` were retired with the + hook: a stamp existed so a per-build hook could report one state once per repository, and a target + a person invokes should answer every time it is asked. Orphan detection is **owner-scoped on purpose**: a lockfile cannot record who asked for an entry, so only an entry under an owner the project references — but from a repository it no longer references diff --git a/src/src/BuildSdk/Sdk/Props/Defaults.props b/src/src/BuildSdk/Sdk/Props/Defaults.props index ccf2d87..b793c16 100644 --- a/src/src/BuildSdk/Sdk/Props/Defaults.props +++ b/src/src/BuildSdk/Sdk/Props/Defaults.props @@ -63,22 +63,11 @@ true true - - true - - false - - + diff --git a/src/src/BuildSdk/Sdk/Sdk.props b/src/src/BuildSdk/Sdk/Sdk.props index c005468..77b1666 100644 --- a/src/src/BuildSdk/Sdk/Sdk.props +++ b/src/src/BuildSdk/Sdk/Sdk.props @@ -419,15 +419,6 @@ Installs from an explicit git source (owner/repo, optionally '#ref') instead of from referenced packages. - - When true, an ordinary build reports missing or version-drifted agent skills. Offline, never installs, never fails the build. Defaults to true. - - - When true, skill recommendations are reported on CI builds as well. Defaults to false. - - - Overrides the stamp used to report each distinct skill state once per repository. Defaults to the repository .purview/skill-recommendations.cache file. - Overrides where the skills CLI's skills-lock.json is read from. Defaults to the repository root. diff --git a/src/src/BuildSdk/Sdk/Sdk.targets b/src/src/BuildSdk/Sdk/Sdk.targets index e6da7de..1094c3f 100644 --- a/src/src/BuildSdk/Sdk/Sdk.targets +++ b/src/src/BuildSdk/Sdk/Sdk.targets @@ -583,8 +583,6 @@ - - @@ -595,7 +593,6 @@ - @@ -875,59 +872,9 @@ Satisfied = satisfied.ToArray(); Orphaned = orphaned.ToArray(); - // ---- Deduplicate ------------------------------------------------------------- - // A solution has many projects and they share one repository, so the same advice - // would otherwise be printed once per project, and again on every rebuild. The - // stamp holds a fingerprint of the exact state last reported: advice repeats only - // when the state actually changes. - var fingerprint = new StringBuilder(); - foreach (var pair in new[] - { - new KeyValuePair>("missing", missing), - new KeyValuePair>("drifted", drifted), - new KeyValuePair>("unknown", unknown), - new KeyValuePair>("orphaned", orphaned), - }) - { - foreach (var line in pair.Value - .Select(item => pair.Key + "|" + item.GetMetadata("PackageId") + "|" - + item.GetMetadata("PackageVersion") + "|" + item.GetMetadata("Skills") - + item.GetMetadata("Skill") + "|" + item.GetMetadata("RepositoryCommit")) - .OrderBy(line => line, StringComparer.Ordinal)) - { - fingerprint.Append(line).Append('\n'); - } - } - - // An unrestored lockfile is its own state: restoring it must end the advice. - if (LockFileFound && !AgentSkillDirectoriesFound && lockedSource.Count > 0) - fingerprint.Append("needs-restore\n"); - - var current = fingerprint.ToString(); - ShouldReport = true; - - if (RespectStamp && !string.IsNullOrEmpty(StampPath)) - { - var previous = string.Empty; - try { if (File.Exists(StampPath)) previous = File.ReadAllText(StampPath); } - catch (IOException) { } - catch (UnauthorizedAccessException) { } - - ShouldReport = !string.Equals(previous, current, StringComparison.Ordinal); - - // A stamp that cannot be written costs a repeated message, never a failed - // build: a read-only or concurrently-built tree must still build. - if (ShouldReport) - { - try - { - Directory.CreateDirectory(Path.GetDirectoryName(StampPath)); - File.WriteAllText(StampPath, current); - } - catch (IOException) { } - catch (UnauthorizedAccessException) { } - } - } + // No stamp and no fingerprint: this task is reached only by a target a person + // invokes, so reporting every time is the point. The fingerprint existed to stop + // a per-build hook repeating itself once per project; that hook is gone. ]]> @@ -1657,11 +1604,19 @@ + @@ -2303,19 +2258,19 @@ + an unresolved root means the pass simply says nothing. --> <_PurviewSkillRecommendationRoot Condition="'$(_PurviewSkillRecommendationRoot)' == ''" >$(_PurviewSkillsRepositoryRoot) $([MSBuild]::NormalizePath('$(_PurviewSkillRecommendationRoot)', 'skills-lock.json')) - $([MSBuild]::NormalizePath('$(_PurviewSkillRecommendationRoot)', '.purview', 'skill-recommendations.cache')) - - <_PurviewSkillRespectStamp Condition="'$(_PurviewSkillRespectStamp)' == ''" - >false * @@ -2400,21 +2350,21 @@ design forbids, so the referenced package's own commit is named instead. --> @@ -2423,39 +2373,80 @@ experimental because the CLI flags it that way. --> + - - <_PurviewSkillRespectStamp>true - + + + + - - - (); + // + // Each file is read exactly once. Both passes below need its lines, and the SDK's + // own .editorconfig is ~2,700 lines / 95 KB, so reading it twice per project per + // compile meant 190 KB of I/O for the same answer it gave on the previous build. + var chain = new List>(); var directory = ProjectDirectory; while (!string.IsNullOrEmpty(directory) && Directory.Exists(directory)) { var candidate = Path.Combine(directory, ".editorconfig"); if (File.Exists(candidate)) { - chain.Add(candidate); - if (DeclaresRoot(candidate)) - break; + var lines = TryReadAllLines(candidate); + if (lines != null) + { + chain.Add(new KeyValuePair(candidate, lines)); + if (DeclaresRoot(lines)) + break; + } } var trimmed = directory.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); @@ -2731,8 +2730,8 @@ var values = new Dictionary(StringComparer.OrdinalIgnoreCase); var sources = new Dictionary(StringComparer.OrdinalIgnoreCase); - foreach (var file in chain) - MergeEditorConfig(file, values, sources); + foreach (var entry in chain) + MergeEditorConfig(entry.Key, entry.Value, values, sources); string optionValue; if (values.TryGetValue("dotnet_style_require_accessibility_modifiers", out optionValue)) @@ -2809,18 +2808,8 @@ "This repository sets '{0}' to '{1}'. Private instance fields must keep the '_' prefix ('_name', never 'name'); restore the prefix, or set DisablePurviewStylePolicyValidation=true to opt out of this validation.", fieldNamingPrefixKey, fieldNamingPrefix); } - bool DeclaresRoot(string file) + bool DeclaresRoot(string[] lines) { - string[] lines; - try - { - lines = File.ReadAllLines(file); - } - catch (Exception) - { - return false; - } - foreach (var line in lines) { var text = StripComment(line).Trim(); @@ -2841,18 +2830,23 @@ return false; } - void MergeEditorConfig(string file, Dictionary target, Dictionary targetSources) + // Null when the file cannot be read. A file that exists but cannot be read is + // treated as absent rather than as a violation: this validation must never fail + // a build over an I/O error it does not understand. + string[] TryReadAllLines(string file) { - string[] lines; try { - lines = File.ReadAllLines(file); + return File.ReadAllLines(file); } catch (Exception) { - return; + return null; } + } + void MergeEditorConfig(string file, string[] lines, Dictionary target, Dictionary targetSources) + { var section = string.Empty; foreach (var line in lines) { @@ -2917,11 +2911,13 @@ /> + @@ -3212,6 +3208,14 @@ GENERATE ASSEMBLY INFO CLASS Writes a static AssemblyInfo class to the intermediate output path so projects have compile-time access to version, company, etc. + + WriteOnlyWhenDifferent is load-bearing, not tidiness. Both generated + files are compile inputs, so an unconditional rewrite gave them a + fresh timestamp on every build and CoreCompile - whose Inputs are + @(Compile) - could never be up to date. Measured on a 39-project + solution: a no-change rebuild ran 38 Csc invocations totalling 111 s + of a 68 s build. With this one attribute the same rebuild compiles + nothing. ===================================================================== --> @@ -3297,6 +3302,9 @@ namespace $(RootNamespace) referencing several assemblies should not need a using per marker. It is only generated for production projects - a test assembly is the one doing the asserting, so it has nothing to identify. + + WriteOnlyWhenDifferent matters here for the same reason it does in + GenerateAssemblyInfoClassTarget: see the note there. ===================================================================== --> diff --git a/src/tests/BuildSdk.IntegrationTests/BuildSdk.IntegrationTests.csproj b/src/tests/BuildSdk.IntegrationTests/BuildSdk.IntegrationTests.csproj index 9e400b3..33f1265 100644 --- a/src/tests/BuildSdk.IntegrationTests/BuildSdk.IntegrationTests.csproj +++ b/src/tests/BuildSdk.IntegrationTests/BuildSdk.IntegrationTests.csproj @@ -31,6 +31,7 @@ static class SdkPaths } " Overwrite="true" + WriteOnlyWhenDifferent="true" /> diff --git a/src/tests/BuildSdk.IntegrationTests/IncrementalBuildTests.cs b/src/tests/BuildSdk.IntegrationTests/IncrementalBuildTests.cs new file mode 100644 index 0000000..2e87512 --- /dev/null +++ b/src/tests/BuildSdk.IntegrationTests/IncrementalBuildTests.cs @@ -0,0 +1,96 @@ +using Purview.BuildSdk.Harness; +using Purview.BuildSdk.Infra; + +namespace Purview.BuildSdk; + +/// +/// Pins the incrementality contract for the files this SDK generates into every project. +/// +/// +/// Both generated files are added to @(Compile), which is one of CoreCompile's +/// Inputs. Writing either of them unconditionally gives it a fresh timestamp, so the compiler +/// output is never newer than its inputs and a no-change rebuild recompiles the project. That is not +/// a theory: it was measured on a 39-project solution, where a rebuild with no edits ran 38 +/// Csc invocations totalling 111 s of a 68 s build, and the fix was a single attribute on each +/// writer. These tests exist so the attribute cannot quietly go missing again. +/// +sealed class IncrementalBuildTests +{ + const string AssemblyInfoFileName = "GeneratedAssemblyInfo.cs"; + const string AssemblyIdentifierFileName = "GeneratedAssemblyIdentifier.cs"; + + [Test] + public async Task AssemblyInfoClass_IsNotRewrittenWhenItsContentHasNotChanged(CancellationToken cancellationToken) + { + await AssertGeneratedFileIsStable(AssemblyInfoFileName, cancellationToken); + } + + [Test] + public async Task AssemblyIdentifierClass_IsNotRewrittenWhenItsContentHasNotChanged( + CancellationToken cancellationToken + ) + { + await AssertGeneratedFileIsStable(AssemblyIdentifierFileName, cancellationToken); + } + + /// + /// Builds the same unchanged project twice and requires the generated file's timestamp to survive. + /// A rewrite is what breaks incrementality, and a rewrite always moves the timestamp - so the + /// timestamp is a more direct assertion than counting compiler invocations. + /// + static async Task AssertGeneratedFileIsStable(string fileName, CancellationToken cancellationToken) + { + using var harness = await ProjectHarness.CreateAsync( + "IncrementalConsumer", + // The telemetry packages the SDK adds by default are not part of this scenario and would + // need Central Package Management versions the throwaway project does not declare. + extraProps: """ + true + true + """, + cancellationToken: cancellationToken + ); + + var first = await harness.BuildAsync(restore: true, cancellationToken: cancellationToken); + await Assert.That(first.Success).IsTrue().Because(TestHelpers.GenerateError(first.Output, first.Errors)); + + var generatedPath = FindGeneratedFile(harness, fileName); + var generatedContent = await File.ReadAllTextAsync(generatedPath, cancellationToken); + var stampedAt = File.GetLastWriteTimeUtc(generatedPath); + + // Long enough that a rewrite would land on a visibly different timestamp, short enough not + // to slow the suite down. Filesystems with coarse timestamps are exactly what this guards + // against, so the delay cannot be removed. + await Task.Delay(TimeSpan.FromMilliseconds(50), cancellationToken); + + var second = await harness.BuildAsync(cancellationToken: cancellationToken); + await Assert.That(second.Success).IsTrue().Because(TestHelpers.GenerateError(second.Output, second.Errors)); + + await Assert + .That(File.GetLastWriteTimeUtc(generatedPath)) + .IsEqualTo(stampedAt) + .Because( + $"'{fileName}' is a compile input, so rewriting it when nothing changed recompiles the " + + "project on every build - WriteOnlyWhenDifferent=\"true\" is what prevents that" + ); + + // The content must still be the real thing: an unconditional *skip* would satisfy the + // timestamp assertion while generating nothing. + await Assert.That(await File.ReadAllTextAsync(generatedPath, cancellationToken)).IsEqualTo(generatedContent); + } + + static string FindGeneratedFile(ProjectHarness harness, string fileName) + { + var matches = Directory.GetFiles( + Path.Combine(harness.ProjectDirectory, "obj"), + fileName, + SearchOption.AllDirectories + ); + + return matches.Length == 1 + ? matches[0] + : throw new InvalidOperationException( + $"Expected exactly one '{fileName}' beneath '{harness.ProjectDirectory}/obj', found {matches.Length}." + ); + } +} diff --git a/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs b/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs index 11f36e5..ffb09e3 100644 --- a/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs @@ -222,11 +222,11 @@ await File.WriteAllTextAsync( /// the pack-time stamp rewrites it and where the SDK later looks for it. /// /// - /// The version in the tracked file is deliberately not asserted against package.json. - /// Packing stamps the version in (see StampPurviewEditorConfigVersionTarget), so requiring the two - /// to match by hand would reintroduce exactly the manual step the stamp removes — and a mismatch here - /// has no effect on what ships. What matters is that the marker exists and is well formed; without it - /// the stamp fails the build rather than packing an unrecognisable file. + /// The format is what this test guards, because the pack-time stamp rewrites the line by + /// matching that prefix - a malformed marker fails the stamp rather than packing an unrecognisable + /// file. The version on it is kept current by just version-sync and asserted by + /// , so a repository reading its own tracked + /// .editorconfig is never shown a release-old number. /// [Test] public async Task EditorConfigVersionMarker_IsTheFirstLineAndWellFormed(CancellationToken cancellationToken) @@ -338,6 +338,7 @@ CancellationToken cancellationToken [Arguments("README.md")] [Arguments("docs/wiki/Getting-Started.md")] [Arguments("templates/global.json")] + [Arguments(".editorconfig")] public async Task AdoptionPin_MatchesPackageJson(string relativePath, CancellationToken cancellationToken) { var path = Path.Combine(RepositoryRoot, relativePath.Replace('/', Path.DirectorySeparatorChar)); diff --git a/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs b/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs index a64d628..42f3983 100644 --- a/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs @@ -20,15 +20,12 @@ sealed class PurviewSkillRecommendationTests /// /// Drives a recommendation target against a fabricated restore graph. The assets file and package /// folder are supplied explicitly so no real restore is needed, and the repository root is pinned - /// to the harness solution so the lockfile and the deduplication stamp stay inside the throwaway - /// tree. + /// to the harness solution so the lockfile stays inside the throwaway tree. /// /// /// BuildingProject is supplied because MSBuild only sets it when the Build target - /// actually runs; invoking a target by name leaves it false. Passing the value a real - /// build would have is what lets these scenarios exercise the rest of the build-hook gating in - /// isolation. That the hook genuinely fires on an ordinary build is covered separately by - /// . + /// actually runs; invoking a target by name leaves it false. These targets no longer gate + /// on it, but supplying it keeps the invocation shaped like a real build. /// static async Task<(int Code, string Output)> RecommendAsync( ProjectHarness harness, @@ -292,125 +289,48 @@ await Assert } [Test] - public async Task TheBuildHook_ReportsEachDistinctStateOnce(CancellationToken cancellationToken) + public async Task VerifyPurviewSkills_FailsWhenSkillsAreMissing(CancellationToken cancellationToken) { using var harness = await ProjectHarness.CreateAsync("SkillConsumer", cancellationToken: cancellationToken); GivenPackageShipping(harness, "purview-alpha"); - var first = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - cancellationToken: cancellationToken - ); - var second = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - cancellationToken: cancellationToken - ); - - await Assert.That(first.Output).Contains("PRSKL0010"); - - // A solution has many projects sharing one repository, and a rebuild changes nothing: the - // A solution has many projects sharing one repository, and a rebuild changes nothing: the - // same advice must not be repeated until the state it describes actually changes. - await Assert.That(second.Output).DoesNotContain("PRSKL0010"); - await Assert - .That(File.Exists(Path.Combine(harness.SolutionDirectory, ".purview", "skill-recommendations.cache"))) - .IsTrue(); + var (code, output) = await RecommendAsync(harness, "VerifyPurviewSkills", cancellationToken: cancellationToken); - // The explicit target answers the question whenever it is asked, stamp or not. - var explicitRun = await RecommendAsync(harness, cancellationToken: cancellationToken); - await Assert.That(explicitRun.Output).Contains("PRSKL0010"); + // This is the gate, not the nag. It is only ever reached because a person or a pipeline + // named it, so escalating a missing skill to a failure breaks nobody by surprise. + await Assert.That(code).IsNotZero().Because(output); + await Assert.That(output).Contains("PRSKL0010"); + await Assert.That(output).Contains("purview-alpha"); } [Test] - public async Task TheBuildHook_ReportsAgainWhenTheStateChanges(CancellationToken cancellationToken) + public async Task VerifyPurviewSkills_FailsWhenTheInstalledSkillIsDrifted(CancellationToken cancellationToken) { using var harness = await ProjectHarness.CreateAsync("SkillConsumer", cancellationToken: cancellationToken); GivenPackageShipping(harness, "purview-alpha"); + GivenSkillsRestored(harness); + SkillPackageFixtures.WriteLockFile(LockFilePath(harness), ("purview-alpha", Source, OtherCommit)); - await RecommendAsync(harness, "RecommendPurviewSkillsOnBuild", cancellationToken: cancellationToken); - - // A second package starts shipping skills: a different state, so a fresh report. - var packagesRoot = Path.Combine(harness.SolutionDirectory, "packages"); - SkillPackageFixtures.WritePackage( - packagesRoot, - "Purview.Second", - "2.0.0", - ["purview-second"], - "https://github.com/purview-dev/second", - "2222222222222222222222222222222222222222" - ); - SkillPackageFixtures.WriteAssetsFile( - Path.Combine(harness.SolutionDirectory, "obj", "project.assets.json"), - (PackageId, PackageVersion), - ("Purview.Second", "2.0.0") - ); - - var (_, output) = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - cancellationToken: cancellationToken - ); - - await Assert.That(output).Contains("purview-second"); - } - - [Test] - public async Task TheBuildHook_StaysSilentOnCI(CancellationToken cancellationToken) - { - using var harness = await ProjectHarness.CreateAsync("SkillConsumer", cancellationToken: cancellationToken); - GivenPackageShipping(harness, "purview-alpha"); + var (code, output) = await RecommendAsync(harness, "VerifyPurviewSkills", cancellationToken: cancellationToken); - // CI installs nothing and has nobody to act on the advice. - var suppressed = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - "-p:ContinuousIntegrationBuild=true", - cancellationToken - ); - await Assert.That(suppressed.Output).DoesNotContain("PRSKL"); - - // A pipeline that deliberately verifies skill state can opt back in. - var opted = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - "-p:ContinuousIntegrationBuild=true -p:PurviewSkillRecommendationsInCI=true", - cancellationToken - ); - await Assert.That(opted.Output).Contains("PRSKL0010"); + await Assert.That(code).IsNotZero().Because(output); + await Assert.That(output).Contains("PRSKL0011"); } [Test] - public async Task TheBuildHook_StaysSilentDuringADesignTimeBuild(CancellationToken cancellationToken) + public async Task VerifyPurviewSkills_PassesWhenTheInstalledSkillMatchesTheReferencedCommit( + CancellationToken cancellationToken + ) { using var harness = await ProjectHarness.CreateAsync("SkillConsumer", cancellationToken: cancellationToken); GivenPackageShipping(harness, "purview-alpha"); - - // An IDE runs design-time builds constantly; none of them is a moment to advise anybody. - var (code, output) = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - "-p:DesignTimeBuild=true", - cancellationToken + GivenSkillsRestored(harness); + SkillPackageFixtures.WriteLockFile( + LockFilePath(harness), + ("purview-alpha", Source, SkillPackageFixtures.DefaultCommit) ); - await Assert.That(code).IsZero().Because(output); - await Assert.That(output).DoesNotContain("PRSKL"); - } - - [Test] - public async Task Recommendations_CanBeTurnedOff(CancellationToken cancellationToken) - { - using var harness = await ProjectHarness.CreateAsync("SkillConsumer", cancellationToken: cancellationToken); - GivenPackageShipping(harness, "purview-alpha"); - - var (code, output) = await RecommendAsync( - harness, - "RecommendPurviewSkillsOnBuild", - "-p:EnablePurviewSkillRecommendations=false", - cancellationToken - ); + var (code, output) = await RecommendAsync(harness, "VerifyPurviewSkills", cancellationToken: cancellationToken); await Assert.That(code).IsZero().Because(output); await Assert.That(output).DoesNotContain("PRSKL"); @@ -454,7 +374,7 @@ public async Task Recommendations_WorkWithoutAnyNodeTooling(CancellationToken ca } [Test] - public async Task AnOrdinaryBuild_ReportsSkillState(CancellationToken cancellationToken) + public async Task AnOrdinaryBuild_ReportsNoSkillState(CancellationToken cancellationToken) { using var harness = await ProjectHarness.CreateAsync( "SkillConsumer", @@ -466,23 +386,23 @@ public async Task AnOrdinaryBuild_ReportsSkillState(CancellationToken cancellati ); // An AGENTS.md beside the solution is what makes the throwaway tree look like a repository: - // the hook resolves its root by the same walk-up the rest of the SDK uses, and refuses to - // fall back to the project folder. + // the skill targets resolve their root by the same walk-up the rest of the SDK uses. await File.WriteAllTextAsync( Path.Combine(harness.SolutionDirectory, "AGENTS.md"), "# Test repository\n", cancellationToken ); - // A committed lockfile with nothing restored beside it needs no skill-shipping package to - // detect, which makes it the one state a genuine end-to-end build can be checked against. + // A committed lockfile with nothing restored beside it is the state that used to produce + // PRSKL0014 on every interactive build. An ordinary build must now say nothing at all: + // the skills pass is explicit, and a build never reads the restore graph for it. SkillPackageFixtures.WriteLockFile( Path.Combine(harness.SolutionDirectory, "skills-lock.json"), ("purview-alpha", Source, SkillPackageFixtures.DefaultCommit) ); - // Verbose, because a recommendation is a high-importance message and the harness builds quiet - // by default - at quiet verbosity an assertion about messages would pass for the wrong reason. + // Verbose, because at quiet verbosity an assertion about messages would pass for the wrong + // reason. var (success, output, errors) = await harness.BuildAsync( restore: true, verbose: true, @@ -490,7 +410,10 @@ await File.WriteAllTextAsync( ); await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); - await Assert.That(output + errors).Contains("PRSKL0014"); + await Assert + .That(output + errors) + .DoesNotContain("PRSKL") + .Because("an ordinary build must not run the skills pass at all"); } [Test] diff --git a/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs b/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs index d7c269e..d408ed2 100644 --- a/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs @@ -59,6 +59,45 @@ public async Task EditorConfigBootstrap_ExistingFile_IsNeverOverwritten(Cancella await Assert.That(await File.ReadAllTextAsync(editorConfigPath, cancellationToken)).IsEqualTo("# user owned\n"); } + /// + /// A design-time build writes nothing. It produces no build output, and an IDE runs one for every + /// edit, so repeating the repository-root probe ladder and two 95 KB reads per project per pass is + /// pure overhead. The next real build bootstraps the files, and external tooling reads them when + /// it runs rather than when the IDE re-evaluates. + /// + [Test] + public async Task DesignTimeBuild_DoesNotBootstrapTheRepositoryFiles(CancellationToken cancellationToken) + { + using var h = await CreateHarnessAsync(extraProps: null, cancellationToken: cancellationToken); + + var editorConfigPath = Path.Combine(h.SolutionDirectory, ".editorconfig"); + var globalJsonPath = Path.Combine(h.SolutionDirectory, "global.json"); + + // A real build first: it bootstraps both files and, just as importantly, restores so the + // design-time pass below has an assets file to read. + var (firstSuccess, firstOutput, firstErrors) = await h.BuildAsync( + restore: true, + verbose: true, + cancellationToken + ); + await Assert.That(firstSuccess).IsTrue().Because(TestHelpers.GenerateError(firstOutput, firstErrors)); + await Assert.That(File.Exists(editorConfigPath)).IsTrue().Because("a real build bootstraps it"); + await Assert.That(File.Exists(globalJsonPath)).IsTrue().Because("a real build bootstraps it"); + + // Remove them, so the design-time pass has something it could do and visibly does not. + File.Delete(editorConfigPath); + File.Delete(globalJsonPath); + + var (code, output, errors) = await h.RunMSBuildAsync( + "-t:Build -p:DesignTimeBuild=true -p:SkipCompilerExecution=true", + cancellationToken + ); + + await Assert.That(code).IsZero().Because(output + errors); + await Assert.That(File.Exists(editorConfigPath)).IsFalse(); + await Assert.That(File.Exists(globalJsonPath)).IsFalse(); + } + /// /// A .editorconfig carrying the SDK's version marker is one the SDK wrote, so a version change /// refreshes it. This is what stops a consuming repository silently running a release-old rule set. diff --git a/templates/global.json b/templates/global.json index 1614832..afbf464 100644 --- a/templates/global.json +++ b/templates/global.json @@ -3,6 +3,6 @@ "runner": "Microsoft.Testing.Platform" }, "msbuild-sdks": { - "Purview.BuildSdk": "1.2.0" + "Purview.BuildSdk": "1.3.0" } }