diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index 2bb4b4b..8881711 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -10,14 +10,14 @@ "rollForward": false }, "dotnet-inspect": { - "version": "0.23.0", + "version": "0.26.0", "commands": [ "dotnet-inspect" ], "rollForward": false }, "purview.build": { - "version": "0.5.3", + "version": "0.6.1", "commands": [ "purview-build" ], diff --git a/.config/purview-build.json b/.config/purview-build.json index af290e6..8f2fc36 100644 --- a/.config/purview-build.json +++ b/.config/purview-build.json @@ -3,7 +3,7 @@ "Solution": "src/BuildSdk.slnx", "TestRoot": "src/tests", "TestPatterns": "*Tests.csproj", - "TestFilter": "/*/*/*/*[Category=Unit]/" + "TestFilter": "/*/*/*/*/" }, "PackValidation": { "RequiredCompilerFlags": [ diff --git a/.csharpierignore b/.csharpierignore deleted file mode 100644 index 786c06d..0000000 --- a/.csharpierignore +++ /dev/null @@ -1,3 +0,0 @@ -**/bin/ -**/obj/ -**/generated/** diff --git a/.editorconfig b/.editorconfig index 120d509..848ca42 100644 --- a/.editorconfig +++ b/.editorconfig @@ -1,3 +1,23 @@ +# Purview.BuildSdk 1.0.7 - 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 +# yours, and is never touched. +# +# To add your own rules, create a second .editorconfig in a subdirectory - `src/.editorconfig` is the +# usual choice - without `root = true`. EditorConfig applies the nearest file last, so your settings win +# over these, and nothing you write is lost when this file is updated. +# +# To stop the SDK updating this file, set one of: +# false keep this file as it is +# Never opt out of all bootstrapping +# false +# +# Deleting the first line also works: without the marker the SDK treats the file as yours. You then own +# every rule in it, including the ones a future SDK version would have fixed. +# +# See https://github.com/purview-dev/build-sdk#editorconfig for the full description. + root = true # Base Configuration - General Settings, File Formatting, and Naming Conventions @@ -360,12 +380,8 @@ dotnet_style_require_accessibility_modifiers = omit_if_default:warning # Code-block preferences csharp_prefer_braces = when_multiline:suggestion csharp_prefer_simple_using_statement = false:silent -csharp_prefer_system_threading_lock = true dotnet_style_prefer_collection_expression = true:suggestion -# Enable foreach explicit cast preference -dotnet_style_prefer_foreach_explicit_cast_in_source = when_strongly_typed:warning - # Prefer System.Threading.Lock csharp_prefer_system_threading_lock = true:warning @@ -497,7 +513,7 @@ dotnet_style_prefer_auto_properties = true:warning dotnet_style_prefer_compound_assignment = true:warning dotnet_style_prefer_conditional_expression_over_assignment = true:suggestion dotnet_style_prefer_conditional_expression_over_return = true:suggestion -dotnet_style_prefer_foreach_explicit_cast_in_source = when_strongly_typed +dotnet_style_prefer_foreach_explicit_cast_in_source = when_strongly_typed:warning dotnet_style_prefer_inferred_anonymous_type_member_names = true:warning dotnet_style_prefer_inferred_tuple_names = true:warning dotnet_style_prefer_simplified_boolean_expressions = false:silent diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ef88697..09fb997 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -21,10 +21,13 @@ jobs: strategy: fail-fast: false matrix: + # Every test project under src/tests. A project missing from this list is simply never run, + # so keep it in step with the folder. project: - Analyzers.UnitTests.csproj - Analyzers.IntegrationTests.csproj - BuildSdk.IntegrationTests.csproj + - SkillsTesting.UnitTests.csproj uses: purview-dev/build/.github/workflows/purview-build.yml@main with: run-lint: false diff --git a/AGENTS.md b/AGENTS.md index 4f66632..0c9a710 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,7 +154,7 @@ Note that a target's `Condition` is evaluated *before* its `DependsOnTargets` ru ### Validating skills -`ValidatePurviewSkills` runs `BeforeTargets="Pack"` and checks that every authored skill is well formed, so a malformed skill fails in the repository that owns it rather than reaching the CLI, which would silently skip it. It checks: +`ValidatePurviewSkills` runs `BeforeTargets="Pack"` and checks that every authored skill is well formed, so a malformed skill fails in the repository that owns it rather than reaching the CLI, which would silently skip it. It validates the folder under the root `ResolvePurviewSkillsRepositoryRoot` resolved — the same one install and recommendation use — rather than the evaluation-time `PurviewSkillsSourceFolder`, because that guess only knows `RepoRoot` and an `AGENTS.md` walk-up: on a CI runner `RepoRoot` comes from `GITHUB_WORKSPACE`, so reading it there validated a different repository's skills and passed silently. It checks: - every folder under the skills root has a `SKILL.md`, - that file opens with YAML frontmatter carrying `name` and `description` (a `>-` block scalar counts), @@ -326,6 +326,10 @@ Use `src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.cs` when validat - `GetItemIdentitiesAsync` / `GetProjectReferencesAsync` - `GetPreprocessProjectAsync` for evaluated project inspection - Use `BuildAsync(restore: true)` when package restore/build behaviour is part of the scenario. +- Spawn processes through `Infra/ProcessRunner` (or `ProjectHarness.RunProcessAsync`), never `new Process`. + It strips the CI runner's environment — `GITHUB_WORKSPACE` moves the SDK's idea of the repository root, + and `CI`/`GITHUB_ACTIONS`/`TF_BUILD` change what is suppressed — so a test cannot pass locally and fail + on `ubuntu-latest`. Use `RunWithEnvironmentAsync` only to opt one variable back in deliberately. - Keep scenarios minimal and deterministic; isolate one behaviour per test. - For import-time behaviour, set required pre-import properties in harness setup (before `Sdk.props` import). diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..7c7ef75 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,218 @@ +# Changelog + +All notable changes to `Purview.BuildSdk` and `Purview.Skills.Testing`. + +The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Versions come from the +`version` field in `package.json`, which is the single source of truth: a merge to `main` publishes +that version to nuget.org. + +This file was reconstructed from the conventional-commit history at 1.0.7, so entries before that +release summarise what the commits recorded rather than what was written at the time. + +## [Unreleased] + +### Added + +- **`CA1034` is suppressed on an `extension` block (`PDS0007`).** A C# 14 extension block is compiled + into a nested type inside its owning static class, so `CA1034` ("nested types should not be visible") + fires on every one, reported at the `extension` keyword and naming nothing because the generated type + has no source name. The nesting is the language's own encoding of the feature rather than a design + choice, and `CA1034` ships at `error` in the shipped `.editorconfig`, so a single extension block + failed the build. The suppression is narrow: only the block's own declaration is suppressed, and an + ordinary nested type keeps the warning whether it sits beside the block or inside it. Add `PDS0007` to + `NoWarn` to restore `CA1034` for every extension block. + +- **An unmarked repository `.editorconfig` is now reported instead of being silently abandoned.** A + repository that adopted the SDK before the version marker existed has a `.editorconfig` with no marker + on its first line, which the refresh deliberately treats as the developer's own file — so it was never + updated and nothing said so, leaving the repository pinned to a release-old rule set indefinitely. + Overwriting it is not safe (nothing on disk distinguishes it from a hand-authored file), so the SDK + warns once per repository per SDK version, naming `-p:PurviewRepoBootstrapMode=Always` as the one-command + fix. The stamp lives at `.purview/editorconfig-adoption.cache` (`PurviewEditorConfigAdoptionStampPath`), + CI builds are skipped so advice can never fail a `-warnaserror` build + (`PurviewEditorConfigAdoptionWarningInCI=true` opts in), and either + `PurviewEditorConfigAdoptionWarning=false` or the existing `PurviewEditorConfigAutoUpdate=false` + silences it. + +### Fixed + +- **Pack-time skill validation now validates the repository it is packing.** `ValidatePurviewSkills` read + the evaluation-time `PurviewSkillsSourceFolder`, which is resolved from `RepoRoot` and an `AGENTS.md` + walk-up. On a CI runner `RepoRoot` is taken from `GITHUB_WORKSPACE`, and it can also arrive stale from + the version-detection cache, so `pack` could validate a *different* repository's `skills/` folder — + passing silently while the packed repository's malformed skill went unnoticed. The target now resolves + the same root every other skill target uses (`ResolvePurviewSkillsRepositoryRoot`), so a malformed skill + fails `pack` in its own repository whatever the ambient environment says. + +## [1.1.0] - 2026-10-21 + +### Performance + +- **The repository bootstrap no longer runs its inline task when there is nothing to do.** + `SyncPurviewRepositoryFiles` is a `RoslynCodeTaskFactory` task, so MSBuild compiles it on first use — + measured at **~724 ms per build** (3,490 ms with the task skipped against 4,214 ms with it forced, on a + real project, median of 5). That was paid on every build of every project even when the task went on to + decide the file was already correct. The "is there anything to do" decision now happens in MSBuild + property functions, so an established repository never pays it. `Always` and `WarnOnDrift` still take the + full path, as does a missing destination. +- **`PurviewRepositoryRootStrategy` replaces the unconditional `git` call.** Finding the repository root + used to spawn `git rev-parse` for every project on every build. The new default, `Markers`, walks up for + the nearest `.git/` using plain MSBuild path checks — no process. Set `PurviewRepositoryRootStrategy=Git` + to restore the old behaviour; the two can disagree, because git reports the *worktree* root, so a + submodule, a linked worktree, or a package nested inside a larger repository may resolve differently. + +### Changed + +- **`CentralPackageTransitivePinningEnabled` now defaults to `true`**, alongside the existing + `ManagePackageVersionsCentrally` default. A transitive dependency can no longer resolve to a version + the repository never named: a central pin below what a transitive requires fails restore with + `NU1109` instead of resolving upward silently. Consumers may need to raise a pin on upgrade — + `aspire-resourcekit` needed OpenTelemetry 1.20.0 → 1.20.1, required by `TUnit.OpenTelemetry`. +- **Every `PackageVersion` names a version; the floating `*` entries are gone.** A `PackageVersion` is + already a minimum — NuGet resolves it unless something in the graph requires higher — so floating was + never needed to stay current, and it made the packed result depend on the day it was built. + `Purview.Skills.Testing` ships `TUnit.Core` and `TUnit.Assertions` as dependencies, so the floor it + declared moved from 1.75.0 to 1.76.0 between 1.0.6 and 1.1.0 purely because time passed. The TUnit + baseline is now `1.76.0`, chosen in `Directory.Packages.props` rather than inherited from a restore. + `CentralPackageFloatingVersionsEnabled` is no longer needed and has been removed. +- The starter `templates/Directory.Packages.props` names versions for the same reason, and referenced + `Microsoft.SourceLink.AzureDevOps.Git`, which does not exist on nuget.org — the package is + `Microsoft.SourceLink.AzureRepos.Git`. The floating version hid it, because nothing ever restored it. +- **CI runs every test suite, not just the unit ones.** `Build:TestFilter` was + `/*/*/*/*[Category=Unit]/`, and the SDK derives an assembly-level TUnit category from each project's name + suffix — so `BuildSdk.IntegrationTests` was `Category=Integration` and its 416 tests, the repository's + main safety net, never ran on a PR. The filter is now `/*/*/*/*/`. +- **`SkillsTesting.UnitTests` is in the PR matrix.** It was absent from `pr.yml` entirely, so its 47 tests + ran nowhere in CI regardless of the filter. The matrix now lists every project under `src/tests`. + + Together these take the PR run from the unit suites alone to all 593 tests. + +### Added + +- `PDS0006`, a diagnostic suppressor for `CA1815` ("override `Equals` and the equality operators on value + types") on union declarations. A union is a value type, so `CA1815` fires on every one, but C# does not + synthesise value equality for a union and a union is read by matching its active case rather than by + comparison — so the rule asks for members a union cannot meaningfully provide. Analyser support for unions + is planned for .NET 12; until then this removes the need for a `#pragma warning disable CA1815` beside every + union declaration. Both the `union` keyword and `System.Runtime.CompilerServices.UnionAttribute` are + recognised. A union's case types, nested types and every other struct keep the warning. Add `PDS0006` to + `NoWarn` to turn it off. + +### Added + +- **The bootstrapped `.editorconfig` carries a version marker and updates itself.** The first line is + `# Purview.BuildSdk - managed file, do not edit`, and a file carrying it is refreshed when the + SDK version changes, so a repository never silently runs a release-old rule set. A `.editorconfig` + *without* the marker belongs to the developer and is never touched — deleting the first line is a + supported way to take ownership. Add your own rules in a second `.editorconfig` in a subdirectory + (`src/.editorconfig`, no `root = true`): EditorConfig applies the nearest file last, so your settings win + and survive every update. Turn the refresh off with `PurviewEditorConfigAutoUpdate=false`, or opt out + entirely with `BootstrapEditorConfigToRepoRoot=false` / `PurviewRepoBootstrapMode=Never`. + + The version in the marker is **stamped at build and pack time** from the version being packed, so it can + never disagree with the package a consumer restored and is not maintained by hand. The stamped copy is + written to `obj/` and packed from there; the tracked `.editorconfig` is never rewritten by a build, which + would otherwise leave a dirty working tree on CI and race when projects build in parallel. The stamp + fails the build if the source file has lost its marker, because a packed file without one would never be + recognised as SDK-managed in a consuming repository. + +### Fixed + +- **`.editorconfig`: `dotnet_style_prefer_foreach_explicit_cast_in_source` was set twice in overlapping + sections** — `when_strongly_typed:warning` under `[*.{cs,csx,cake}]` and `when_strongly_typed` (no + severity) under `[*.{cs,csx,cake,vb,vbx}]`. Both match a `.cs` file and the later section wins, so the + intended `warning` severity was silently discarded for every C# file. Now declared once, with the + severity, in the broader section. +- **`.editorconfig`: `csharp_prefer_system_threading_lock` was declared twice** in the same section, the + first without a severity. The dead declaration is gone. +- `SplitExtensionsClassCodeRefactoringProvider` built its new documents with `NormalizeWhitespace()`, which + imposes Roslyn's own defaults — four spaces, Allman braces — and so ignored the `.editorconfig` this SDK + ships and bootstraps into every consuming repository. A split or move produced a file that failed that + repository's own formatting check the moment it was created. The generated syntax now carries + `Formatter.Annotation` and is formatted against the new document's real options. +- The same refactoring threw `InvalidOperationException` when a method symbol had no resolvable declaration + — a method from metadata, or from a partial compilation the IDE is still building. That takes down the + light-bulb for the whole refactoring with an exception the developer can neither read nor act on. Such a + method is now skipped, and a receiver left with no resolvable methods is skipped entirely. + +### Performance + +- `Sdk.props` now reads the project file once and strips its XML comments once, sharing the result + through `_PurviewProjectText`. The six project-text scans (`IsRoslynComponent`, + `IsRoslynComponentOnly`, `IsPackable`, `TargetFramework(s)`, `RootNamespace`, + `PurviewTargetFrameworkSet`) each used to re-read and re-strip the file, so every evaluation of + every project paid six times over. Measured at ~0.59 ms saved per project, ~117 ms on a 200-project + solution — see [`docs/wiki/Performance.md`](docs/wiki/Performance.md). + +### Fixed + +- Stray trailing space in the `Props/VersionDetection.props` import path in `Sdk.props`. +- `PurviewBuildSdkVersionForGlobalJson` fell back to `1.0.0`, so a bootstrapped `global.json` pinned a + release that predated every fix in the SDK doing the bootstrapping. It now tracks `package.json`, and + `PackagedVersionFallbackTests` fails the build if the two drift apart again. + +### Documentation + +- The quick-start `global.json` snippets in `README.md` and `docs/wiki/Getting-Started.md`, and + `templates/global.json`, pinned `1.0.0` while 1.0.7 shipped. Now pinned to the current version and + covered by `PackagedVersionFallbackTests`. +- `README.md` and `docs/wiki/Getting-Started.md` told adopters to copy `templates/Directory.Packages.props` + "from this package". `templates/` is not packed; both now link to the file in git and say so. +- `docs/wiki/Home.md` still described the build-time `.agents/**` mirror, which was removed outright in + 1.0.6. Replaced with the skills-CLI behaviour that superseded it. +- The analyzer rule table in `README.md` omitted `PDS0005`, and `docs/wiki/Home.md` described + `PDS0001`–`PDS0005` as analyzers. `PDS0001` and `PDS0005` are `DiagnosticSuppressor`s, which is why + they are absent from `AnalyzerReleases.Shipped.md`; both documents now say so. +- Added `docs/wiki/Performance.md`. + +## [1.0.7] + +### Changed + +- Moved `Purview.Architecture.Testing` out to `purview-dev/architests`. + +## [1.0.6] + +### Added + +- Agent skills resolved and installed explicitly, delegating to the skills CLI; authored skills are + validated at pack time. +- Assembly name and identity generation. + +### Changed + +- Migrated to the skills CLI as the single installer; the build-time `.agents/**` mirror was removed. +- Architecture discovery now fails loudly instead of silently. +- `RequiredCompilerFlags` declared explicitly; `Purview.Build` installed as a .NET local tool. + +### Fixed + +- A packing issue. + +## [1.0.5] + +### Documentation + +- Corrected the test-detection and namespace-stripping claims. + +## [1.0.4] + +### Added + +- Curated target-framework sets. +- Roslyn-component-only project support. +- Additional testing types and namespace prefixes. + +### Fixed + +- The single-TFM floor shadowing a project's `TargetFrameworks`. +- Target frameworks resolving empty under some conditions. +- `.editorconfig` values missing test projects. +- Source-generator resolution on pack. + +[Unreleased]: https://github.com/purview-dev/build-sdk/compare/v1.1.0...HEAD +[1.1.0]: https://github.com/purview-dev/build-sdk/compare/v1.0.7...v1.1.0 +[1.0.7]: https://github.com/purview-dev/build-sdk/compare/v1.0.6...v1.0.7 +[1.0.6]: https://github.com/purview-dev/build-sdk/compare/v1.0.5...v1.0.6 +[1.0.5]: https://github.com/purview-dev/build-sdk/compare/v1.0.4...v1.0.5 +[1.0.4]: https://github.com/purview-dev/build-sdk/releases/tag/v1.0.4 diff --git a/Directory.Packages.props b/Directory.Packages.props index e2a51ce..7a2f40c 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -1,25 +1,30 @@ - true + 1.76.0 + 5.9.0 - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + + diff --git a/README.md b/README.md index eb3d1de..9549048 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ A reusable MSBuild SDK NuGet package that delivers standardised .NET project def "runner": "Microsoft.Testing.Platform" }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.0" + "Purview.BuildSdk": "1.1.0" } } ``` @@ -69,7 +69,7 @@ A reusable MSBuild SDK NuGet package that delivers standardised .NET project def ### 4. Copy `Directory.Packages.props` to repo root -Copy `templates/Directory.Packages.props` from this package to your repo root. All package versions default to `*` (latest at restore). Pin any package by replacing `*` with a specific version. +Copy [`templates/Directory.Packages.props`](https://github.com/purview-dev/build-sdk/blob/main/templates/Directory.Packages.props) from the repository to your repo root. The templates are not packed into the NuGet package — they are starting points held in git, so take them from there. Every package names a version. A `PackageVersion` is a minimum rather than an exact pin — NuGet resolves it unless something in the graph requires higher — so floating versions are not needed to stay current, and they would make a build depend on the day it ran. > **Note:** `ManagePackageVersionsCentrally=true` is set by the SDK. You **must** have a `Directory.Packages.props` at your repo root for CPM to work, even if it only contains the packages the SDK adds automatically. @@ -195,6 +195,84 @@ place, an existing file is never overwritten unless asked, retries are silent (` to a low-importance message) and only a copy that still fails after every retry is reported. See [Repository Bootstrap](docs/wiki/Repository-Bootstrap.md) for the retry and failure properties. +### `.editorconfig` + +The bootstrapped `.editorconfig` opens with a version marker: + +```editorconfig +# Purview.BuildSdk 1.1.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 +the marker is refreshed so a repository never silently runs a release-old rule set. A `.editorconfig` +**without** the marker is yours and is never touched, whatever it contains — which makes deleting the +first line a supported way to take ownership of the file. You then own every rule in it, including the +ones a later SDK version would have fixed. + +A repository that adopted the SDK *before* the marker existed has exactly that file: unmarked, so it +is never refreshed, and indistinguishable on disk from one you wrote. Rather than overwrite it, the +SDK says so once: + +> `/.editorconfig` carries no Purview.BuildSdk version marker, so the SDK treats it as yours and +> will never refresh it … + +Build once with `-p:PurviewRepoBootstrapMode=Always` to adopt the SDK's file, or set +`PurviewEditorConfigAutoUpdate=false` to keep your own and silence it. The warning is reported once +per repository per SDK version — stamped at `.purview/editorconfig-adoption.cache`, overridable with +`PurviewEditorConfigAdoptionStampPath` — and is skipped on CI builds, so no advice can fail a build +run with `-warnaserror`. `PurviewEditorConfigAdoptionWarningInCI=true` opts back in; +`PurviewEditorConfigAdoptionWarning=false` turns it off entirely. + +The version is stamped in when the package is built, from the version being packed, so it is always +the version you restored. It is not maintained by hand in the repository, and building never rewrites +the tracked `.editorconfig`: the stamped copy is written to `obj/` and that copy is packed. + +#### Adding your own rules + +Do not edit the managed file — put your rules in a **second `.editorconfig` in a subdirectory**, most +often `src/.editorconfig`, and omit `root = true`: + +```editorconfig +# src/.editorconfig - yours, never touched by the SDK +[*.cs] +dotnet_diagnostic.CA1062.severity = none +``` + +EditorConfig applies the nearest file last, so your settings override the SDK's for everything beneath +that folder, and nothing you write is lost when the managed file is updated. A rule you want to turn +off repository-wide is better expressed as `NoWarn` in `Directory.Build.props`, which the managed file +also never touches. + +#### Turning the update off + +| Property | Effect | +| -- | -- | +| `PurviewEditorConfigAutoUpdate=false` | Keep the managed `.editorconfig` exactly as it is; everything else still bootstraps. | +| `PurviewEditorConfigAdoptionWarning=false` | Keep an unmarked `.editorconfig` without being told it will never be refreshed. | +| `BootstrapEditorConfigToRepoRoot=false` | Do not write a repository `.editorconfig` at all. | +| `PurviewRepoBootstrapMode=Never` | Opt out of all bootstrapping, `global.json` included. | + +#### Finding the repository root + +`PurviewRepositoryRootStrategy` chooses how the bootstrap locates the repository root: + +| Value | Behaviour | +| -- | -- | +| `Markers` *(default)* | The nearest ancestor directory containing `.git/`, found with plain MSBuild path checks. No process is started. | +| `Git` | Asks git via `rev-parse`, falling back to the walk. | + +`Markers` is the default because `Git` spawns a git process for every project on every build, whether or +not anything needs bootstrapping. The two can disagree: git reports the *worktree* root, so for a +submodule, a linked worktree, or a package nested inside a larger repository it may answer differently +from "the nearest `.git/` above me". Set `Git` where that distinction matters. + +The bootstrap also skips its file-sync task entirely when there is nothing to do — a destination that +exists, is not marker-stale, and is not being checked for drift. That avoids compiling the inline task, +which is worth roughly 0.7s per build. + +`PurviewRepoBootstrapMode=Always` still forces an overwrite regardless of the marker, and +`WarnOnDrift` still reports a difference without writing. + ### Agent skills Purview skills are published in the standard [Agent Skills](https://skills.sh/) format and installed @@ -698,6 +776,76 @@ copy of the shipped file, a stale copy is reported rather than obeyed: refresh i | `PDS0002` | Naming | Warning | Files under a project-root `Extensions/` folder reset their namespace | | `PDS0003` | Style | Warning | Prefer an explicit type with target-typed `new()` over `var` | | `PDS0004` | Naming | Warning | Use correct acronym capitalization (`Api` → `API`) | +| `PDS0005` | (suppressor) | — | Suppresses `IDE0130` for files under a project-root `Extensions/` folder, whose namespace is reset by `PDS0002` | +| `PDS0006` | (suppressor) | — | Suppresses `CA1815` on a union declaration | +| `PDS0007` | (suppressor) | — | Suppresses `CA1034` on an `extension` block | + +`PDS0001`, `PDS0005`, `PDS0006` and `PDS0007` are `DiagnosticSuppressor`s rather than analyzers, so they +declare `SuppressionDescriptor`s and do not appear in `AnalyzerReleases.Shipped.md` — that file tracks +analyzer rules only. + +### Union equality (`PDS0006`) + +A union is a value type, so `CA1815` ("override `Equals` and the equality operators on value types") fires +on every one. The members it asks for cannot sensibly be written: C# does not synthesise value equality for +a union declaration, and a union is read by matching its active case rather than by comparing two unions. +Analyser support for unions is planned for .NET 12, so until then every union declaration would otherwise +need a `#pragma warning disable CA1815` or a `[SuppressMessage]` beside it. + +Both spellings are recognised, because both produce a union and both are reported by `CA1815`: + +```csharp +// the union keyword +public readonly union TenantError(NotFound, Disabled); + +// System.Runtime.CompilerServices.UnionAttribute, which is also how the keyword form is +// encoded in metadata, so a union from a referenced assembly is recognised too +[Union] +public struct AttributedUnion { public int Tag; } +``` + +The suppression is deliberately narrow. A union's **case types** are ordinary value types and keep the +warning, as does every other struct and any type nested inside a union: + +```csharp +public readonly record struct NotFound(int Id); // CA1815 still reported +public readonly union TenantError(NotFound); // CA1815 suppressed +``` + +Add `PDS0006` to `NoWarn` to turn the suppression off, which brings `CA1815` back for every union. + +### Extension block nesting (`PDS0007`) + +A C# 14 `extension` block is compiled into a nested type inside its owning static class, so `CA1034` +("nested types should not be visible") fires on every one — reported at the `extension` keyword and naming +nothing, because the generated type has no source name: + +```text +Lib.cs(5,2): error CA1034: Do not nest type . Alternatively, change its accessibility so that it is not +externally visible. +``` + +The rule's advice cannot be taken: the nesting is the language's own encoding of the feature, not a design +choice. `CA1034` ships at `error` in the shipped `.editorconfig`, so without this suppression a single +extension block fails the build, and the only alternative is a `#pragma warning disable CA1034` or a +`[SuppressMessage]` beside every one. + +The suppression is deliberately narrow — only the extension block's own declaration is suppressed, and an +ordinary nested type keeps the warning whether it sits beside the block or inside it: + +```csharp +public static class StringExtensions +{ + extension(string value) // CA1034 suppressed + { + public bool IsBlank => string.IsNullOrWhiteSpace(value); + } + + public class Options { } // CA1034 still reported +} +``` + +Add `PDS0007` to `NoWarn` to turn the suppression off, which restores `CA1034` for every extension block. `PDS0004` follows .NET naming guidance for well-known framework spellings (`Sql`, `Guid`, `Uuid`, `Url`, `Dns`, `Http`, `Xml`, `DbContext`, ...) while still enforcing uppercase for acronyms such as `Api` → `API`, diff --git a/docs/wiki/Analyzers.md b/docs/wiki/Analyzers.md index a46e58f..fe9e7ae 100644 --- a/docs/wiki/Analyzers.md +++ b/docs/wiki/Analyzers.md @@ -11,6 +11,8 @@ command-line builds and Visual Studio. | `PDS0003` | Style | Warning | Prefer an explicit type with target-typed `new()` over `var` | | `PDS0004` | Naming | Warning | Use correct acronym capitalization (`Api` → `API`) | | `PDS0005` | (suppressor) | — | Suppresses `IDE0130` for files rooted under `Extensions/` | +| `PDS0006` | (suppressor) | — | Suppresses `CA1815` on a union declaration | +| `PDS0007` | (suppressor) | — | Suppresses `CA1034` on an `extension` block | ## Style policy diff --git a/docs/wiki/Getting-Started.md b/docs/wiki/Getting-Started.md index 07c06a1..fe97d3d 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.0.0" + "Purview.BuildSdk": "1.1.0" } } ``` @@ -49,8 +49,12 @@ Add `Purview.BuildSdk` to the `msbuild-sdks` section so MSBuild can resolve the ## 4. Copy `Directory.Packages.props` to the repo root -Copy `templates/Directory.Packages.props` from this package to your repo root. All package versions -default to `*` (latest at restore). Pin any package by replacing `*` with a specific version. +Copy [`templates/Directory.Packages.props`](https://github.com/purview-dev/build-sdk/blob/main/templates/Directory.Packages.props) +from the repository to your repo root. The templates are not packed into the NuGet package — they are +starting points held in git, so take them from there. Every package names a version. A +`PackageVersion` is a minimum rather than an exact pin — NuGet resolves it unless something in the +graph requires higher — so floating versions are not needed to stay current, and they would make a +build depend on the day it ran. > **Note:** `ManagePackageVersionsCentrally=true` is set by the SDK. You **must** have a > `Directory.Packages.props` at your repo root for CPM to work, even if it only contains the packages diff --git a/docs/wiki/Home.md b/docs/wiki/Home.md index 87b27af..c35c6a8 100644 --- a/docs/wiki/Home.md +++ b/docs/wiki/Home.md @@ -52,12 +52,15 @@ repo root inherits everything automatically. default (`DisableAutoCopySdkFiles=true` to opt out). Existing files are never overwritten unless `PurviewRepoBootstrapMode` says otherwise, and a race between parallel projects is a no-op rather than a build failure. -- **Bundled analyzers and code fixes** — `PDS0001`–`PDS0005` plus IDE code fixes for naming and - extensions-namespace conventions. -- **Agent folder** — the package ships `.agents/**` content that is mirrored into the consuming - repository (change-aware, so unchanged files are never rewritten) so coding agents can discover - repository-aware guidance automatically. Copy retries are silent; a copy that still fails after every - retry is reported as an error. +- **Bundled analyzers and code fixes** — three analyzers (`PDS0002`–`PDS0004`) and four suppressors + (`PDS0001`, `PDS0005`, `PDS0006`, `PDS0007`), plus IDE code fixes for naming and extensions-namespace + conventions. +- **Agent skills** — the SDK resolves which skills a restored package ships and the exact commit it was + built from, and reports what is missing or has drifted (`PRSKL0010`–`PRSKL0014`) during an ordinary + build. Installing them is a separate, explicit step (`dotnet msbuild -t:InstallPurviewSkills`) that + delegates to the skills CLI. The SDK itself writes nothing into an agent directory: the build-time + `.agents/**` mirror was removed outright, because the CLI owns those paths and two writers on one set + of paths meant the mirror silently overwriting a pinned install. ## Requirements diff --git a/docs/wiki/Performance.md b/docs/wiki/Performance.md new file mode 100644 index 0000000..57fe7ca --- /dev/null +++ b/docs/wiki/Performance.md @@ -0,0 +1,77 @@ +# Performance + +What this SDK costs at **evaluation** time, and what has been done about it. + +Evaluation cost is the number that matters here. `Sdk.props` is imported by every project in every +consuming repository, and it is evaluated on every build, every restore, and every design-time build the +IDE runs — which, for an open solution, is continuously. + +## Project-text scans + +`Sdk.props` has to inspect the raw project text, because it is imported *before* the project body is +evaluated and so cannot ask MSBuild what the project declares. Six properties are resolved this way: + +| Property | Looks for | +| -- | -- | +| `IsRoslynComponent` | `true` | +| `IsRoslynComponentOnly` | `false` | +| `IsPackable` | `true` | +| `_PurviewProjectDeclaresTargetFramework` | `` / `` | +| `_PurviewProjectDeclaresRootNamespace` | a non-empty `` | +| `_PurviewProjectTargetFrameworkSet` | `` | + +Each of these used to re-read the project file and re-strip its XML comments before running its own +regex, so one evaluation paid for **six** file reads and six full-text comment-stripping replacements. +The file is now read once into `_PurviewProjectRawText`, comment-stripped once into +`_PurviewProjectText`, and all six scans share that result. `SdkProjectName` reuses the raw text. + +### Measured + +Isolating just the read-and-strip work, over `BuildSdk.csproj` (4,756 bytes), 2,000 iterations: + +| | Per project | +| -- | -- | +| Before — 6 reads + 6 comment strips | 0.70 ms | +| After — 1 read + 1 comment strip | 0.12 ms | +| **Saving** | **0.59 ms** | + +Scaled to a solution-wide evaluation: + +| Projects | Saving per evaluation | +| -- | -- | +| 50 | ~29 ms | +| 200 | ~117 ms | + +Be clear about the size of this: it is a real saving and it is paid back on every evaluation, but it is +not transformative. On a 200-project solution it is roughly a tenth of a second against an evaluation +measured in seconds. It was worth doing because the cost was pure waste and the fix is behaviour-free — +not because evaluation was slow. + +### Why the scans were not shared before + +`Sdk.props:184` carries a warning that the project text must never be passed as a **quoted string +literal**, because project text contains commas, parentheses and quotes that break MSBuild's +property-function argument parsing. That warning is correct, and it is a different thing from passing +the text as `$(_PurviewProjectText)`. + +Passing a property reference was verified against MSBuild directly before the change: the property is +expanded into the argument without its content being re-parsed as an argument list, and without any +`$(...)` the project text happens to contain being re-expanded. A project containing +`$(SomeUndefinedProperty)` still matches the literal text through the shared property, +and the property form and the old nested form agree on every probe. + +The regression net is `BuildSdk.IntegrationTests`, which asserts evaluated property values directly +through `ProjectHarness.GetPropertyAsync` against real `dotnet msbuild` invocations. All 406 tests pass +unchanged. + +## Inline tasks + +`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. + +## Not yet measured + +- 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. diff --git a/docs/wiki/Release-Flow.md b/docs/wiki/Release-Flow.md index 0e460f6..a324acf 100644 --- a/docs/wiki/Release-Flow.md +++ b/docs/wiki/Release-Flow.md @@ -41,7 +41,7 @@ The pipeline configuration lives in `purview-build.json`: "Solution": "src/BuildSdk.slnx", "TestRoot": "src/tests", "TestPatterns": "*Tests.csproj", - "TestFilter": "/*/*/*/*[Category=Unit]/" + "TestFilter": "/*/*/*/*/" }, "PackValidation": { "RequireSymbolPackage": false diff --git a/global.json b/global.json index f0bb4a3..dcef14c 100644 --- a/global.json +++ b/global.json @@ -1,8 +1,5 @@ { - "sdk": { - "allowPrerelease": false - }, "test": { "runner": "Microsoft.Testing.Platform" } -} +} \ No newline at end of file diff --git a/package.json b/package.json index 1195360..7d495f9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "build-sdk", - "version": "1.0.7", + "version": "1.1.0", "homepage": "https://purview.dev/projects/build-sdk/", "bugs": { "url": "https://github.com/purview-dev/build-sdk/issues" diff --git a/src/Directory.Build.props b/src/Directory.Build.props index cf06bd3..89b9e2d 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,11 +16,6 @@ - - - - - https://purview.dev/ $(PurviewHomepage)projects/build-sdk/ diff --git a/src/src/Analyzers/ExtensionBlocks/ExtensionBlockNestingSuppressor.cs b/src/src/Analyzers/ExtensionBlocks/ExtensionBlockNestingSuppressor.cs new file mode 100644 index 0000000..b56c7e7 --- /dev/null +++ b/src/src/Analyzers/ExtensionBlocks/ExtensionBlockNestingSuppressor.cs @@ -0,0 +1,133 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.BuildSdk.Analyzers.ExtensionBlocks; + +/// +/// Suppresses CA1034 (nested types should not be visible) on an extension block. +/// +/// +/// +/// A C# 14 extension block is compiled into a nested type inside its owning static class, so +/// CA1034 fires on every one — reported at the extension keyword, naming nothing, because the +/// generated type has no source name ("Do not nest type ."). The rule's advice cannot be taken: the nesting +/// is the language's own encoding of the feature, not a design choice, and the only alternative would be a +/// #pragma warning disable CA1034 or a [SuppressMessage] beside every extension block in every +/// consuming repository. CA1034 ships at error in this SDK's .editorconfig, so without +/// this suppression an extension block does not merely warn, it fails the build. +/// +/// +/// The suppression is deliberately narrow: only the extension block's own declaration is suppressed. An +/// ordinary public type nested inside the same static class keeps the warning, because the innermost +/// declaration enclosing the reported location is the one that decides. +/// +/// +/// A repository that wants the warning back adds PDS0007 to NoWarn, which restores +/// CA1034 for every extension block. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ExtensionBlockNestingSuppressor : DiagnosticSuppressor +{ + /// + /// The syntax kind of an extension block declaration. + /// + /// + /// Compared as a raw kind value rather than by testing the node against + /// ExtensionBlockDeclarationSyntax, so the suppressor still loads on a compiler host whose + /// Microsoft.CodeAnalysis.CSharp predates that type. An enum member is a compile-time constant and + /// carries no runtime dependency; a type test would be resolved when the method is jitted. + /// + const int ExtensionBlockDeclarationKind = (int)SyntaxKind.ExtensionBlockDeclaration; + + /// + /// The suppression of CA1034 on an extension block. + /// + /// + /// This is not a rule and is never reported. It is the id a repository adds to NoWarn to turn the + /// suppression off, and the id every suppression is logged under. Suppression descriptors are not + /// release-tracked, so it must not appear in AnalyzerReleases.Shipped.md, which lists reported + /// rules only. + /// + public static readonly SuppressionDescriptor SuppressNestingOnExtensionBlocks = new( + "PDS0007", + "CA1034", + "An extension block is compiled into a nested type inside its owning static class, so CA1034 reports a nesting the language mandates and the developer cannot remove." + ); + + /// + public override ImmutableArray SupportedSuppressions => [SuppressNestingOnExtensionBlocks]; + + /// + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + foreach (var diagnostic in context.ReportedDiagnostics) + { + // Only CA1034 is declared as suppressible, so this is the same bucket the descriptor names. + if ( + !string.Equals( + diagnostic.Id, + SuppressNestingOnExtensionBlocks.SuppressedDiagnosticId, + StringComparison.Ordinal + ) + ) + { + continue; + } + + if (!IsExtensionBlockDeclaration(diagnostic, context.CancellationToken)) + { + continue; + } + + context.ReportSuppression(Suppression.Create(SuppressNestingOnExtensionBlocks, diagnostic)); + } + } + + /// + /// Determines whether the reported diagnostic sits on an extension block declaration. + /// + /// + /// CA1034 is reported at the extension keyword, so the location is resolved to the nearest + /// enclosing declaration that declares a type. The innermost one wins: an ordinary nested type inside a + /// static class that also holds extension blocks keeps its warning. + /// + static bool IsExtensionBlockDeclaration(Diagnostic diagnostic, CancellationToken cancellationToken) + { + var location = diagnostic.Location; + if (!location.IsInSource || location.SourceTree is not { } tree) + { + return false; + } + + var root = tree.GetRoot(cancellationToken); + var node = root.FindNode(location.SourceSpan, getInnermostNodeForTie: true); + + // Decided from syntax alone, with no semantic model: an extension block is a declaration form, so the + // syntax tree already answers the question, and the generated type carries no name to match against. + for (var current = node; current is not null; current = current.Parent) + { + if (!IsTypeDeclaration(current)) + { + continue; + } + + return current.RawKind == ExtensionBlockDeclarationKind; + } + + return false; + } + + static bool IsTypeDeclaration(SyntaxNode node) => + node.RawKind + is (int)SyntaxKind.ClassDeclaration + or (int)SyntaxKind.StructDeclaration + or (int)SyntaxKind.InterfaceDeclaration + or (int)SyntaxKind.RecordDeclaration + or (int)SyntaxKind.RecordStructDeclaration + or (int)SyntaxKind.EnumDeclaration + or (int)SyntaxKind.DelegateDeclaration + or ExtensionBlockDeclarationKind; +} diff --git a/src/src/Analyzers/Unions/UnionEqualitySuppressor.cs b/src/src/Analyzers/Unions/UnionEqualitySuppressor.cs new file mode 100644 index 0000000..dfa1be0 --- /dev/null +++ b/src/src/Analyzers/Unions/UnionEqualitySuppressor.cs @@ -0,0 +1,149 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +// ITypeSymbol.IsUnion is the language's own answer to "is this a union", and is marked [Experimental] +// until C# unions ship. Targeting it deliberately is the whole point of this suppressor, so the +// experimental warning is disabled here rather than repo-wide. +#pragma warning disable RSEXPERIMENTAL006 + +namespace Purview.BuildSdk.Analyzers.Unions; + +/// +/// Suppresses CA1815 (override Equals and the equality operators on value types) on union +/// declarations. +/// +/// +/// +/// A union is a value type, so CA1815 fires on every one. The members it asks for cannot sensibly be +/// written: C# does not synthesise value equality for a union declaration, and a union is read by matching +/// its active case rather than by comparing two unions. Analyser support for unions is planned for .NET 12, +/// so until then every union declaration in every consuming repository would otherwise need a +/// #pragma warning disable CA1815 or a [SuppressMessage] beside it. +/// +/// +/// The suppression is deliberately narrow: it applies only where the reported declaration is itself a union. +/// A union's **case types** are ordinary value types and keep the warning, as does every other struct — if +/// the reported type is not a union, nothing is suppressed. +/// +/// +/// Both spellings count, because both produce a union and both are reported by CA1815: +/// +/// +/// the union keyword, which answers for; and +/// System.Runtime.CompilerServices.UnionAttribute, which is also how the keyword form is encoded +/// in metadata, so a union read from a referenced assembly is recognised too. +/// +/// +/// A repository that wants union equality declared turns the suppression off by adding PDS0006 to +/// NoWarn, which brings CA1815 back for every union. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class UnionEqualitySuppressor : DiagnosticSuppressor +{ + /// + /// The metadata name of the attribute the compiler applies to a union. + /// + /// + /// Matched by name rather than by symbol identity. The attribute is embedded per-assembly when the + /// targeted framework does not supply it, so a union from a referenced assembly carries that assembly's + /// copy, and would not match it against this compilation's. + /// + const string UnionAttributeMetadataName = "System.Runtime.CompilerServices.UnionAttribute"; + + /// + /// The suppression of CA1815 on a union declaration. + /// + /// + /// This is not a rule and is never reported. It is the id a repository adds to NoWarn to turn the + /// suppression off, and the id every suppression is logged under. Suppression descriptors are not + /// release-tracked, so it must not appear in AnalyzerReleases.Shipped.md, which lists reported + /// rules only. + /// + public static readonly SuppressionDescriptor SuppressEqualityOnUnions = new( + "PDS0006", + "CA1815", + "A union is matched by its active case rather than compared by value, and C# does not synthesise value equality for a union declaration, so CA1815 asks for members a union cannot meaningfully provide. Analyser support for unions is planned for .NET 12." + ); + + /// + public override ImmutableArray SupportedSuppressions => [SuppressEqualityOnUnions]; + + /// + public override void ReportSuppressions(SuppressionAnalysisContext context) + { + foreach (var diagnostic in context.ReportedDiagnostics) + { + // Only CA1815 is declared as suppressible, so this is the same bucket the descriptor names. + if ( + !string.Equals(diagnostic.Id, SuppressEqualityOnUnions.SuppressedDiagnosticId, StringComparison.Ordinal) + ) + { + continue; + } + + if (!IsUnionDeclaration(context, diagnostic)) + { + continue; + } + + context.ReportSuppression(Suppression.Create(SuppressEqualityOnUnions, diagnostic)); + } + } + + /// + /// Determines whether the reported diagnostic sits on a union declaration. + /// + /// + /// CA1815 is reported at the declared type's identifier, so the location is resolved to the nearest + /// enclosing declaration that declares a type. The innermost one wins: a union that nests an ordinary + /// struct does not suppress the nested struct's warning. + /// + static bool IsUnionDeclaration(SuppressionAnalysisContext context, Diagnostic diagnostic) + { + var location = diagnostic.Location; + if (!location.IsInSource || location.SourceTree is not { } tree) + { + return false; + } + + var semanticModel = context.GetSemanticModel(tree); + var root = tree.GetRoot(context.CancellationToken); + var node = root.FindNode(location.SourceSpan, getInnermostNodeForTie: true); + + // Walked with the general GetDeclaredSymbol rather than by matching a syntax type, so the union + // declaration syntax is recognised whatever shape the preview language feature settles on. + for (var current = node; current is not null; current = current.Parent) + { + if (semanticModel.GetDeclaredSymbol(current, context.CancellationToken) is not INamedTypeSymbol type) + { + continue; + } + + return type.IsUnion || HasUnionAttribute(type); + } + + return false; + } + + static bool HasUnionAttribute(INamedTypeSymbol type) + { + foreach (var attribute in type.GetAttributes()) + { + if ( + attribute.AttributeClass is { } attributeClass + && string.Equals( + attributeClass.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), + "global::" + UnionAttributeMetadataName, + StringComparison.Ordinal + ) + ) + { + return true; + } + } + + return false; + } +} diff --git a/src/src/BuildSdk/BuildSdk.csproj b/src/src/BuildSdk/BuildSdk.csproj index 23b1ce4..5dd3e95 100644 --- a/src/src/BuildSdk/BuildSdk.csproj +++ b/src/src/BuildSdk/BuildSdk.csproj @@ -53,16 +53,142 @@ /> + + + <_PurviewSourceEditorConfig>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)../../../.editorconfig')) + + + + + + + + + + + + + + + + + + + + + + <_PurviewEditorConfigStampVersion>$([System.String]::Copy('$(PackageVersion)').Trim()) + <_PurviewEditorConfigStampVersion Condition="'$(_PurviewEditorConfigStampVersion)' == ''" + >$(Version) + + <_PurviewStampedEditorConfig Condition="'$(_PurviewStampedEditorConfig)' == ''" + >$([System.IO.Path]::GetFullPath('$(MSBuildProjectDirectory)/$(IntermediateOutputPath)purview-editorconfig/.editorconfig')) + + + + + + + + + - - true true + true false false @@ -167,8 +169,50 @@ >Library false true + + Markers + + # Purview.BuildSdk + true + + true + false + + 1.0.0 + >1.1.0 diff --git a/src/src/BuildSdk/Sdk/Sdk.props b/src/src/BuildSdk/Sdk/Sdk.props index 61457e5..55fbbbe 100644 --- a/src/src/BuildSdk/Sdk/Sdk.props +++ b/src/src/BuildSdk/Sdk/Sdk.props @@ -1,6 +1,6 @@ - + + <_PurviewProjectRawText>$([System.IO.File]::ReadAllText(`$(MSBuildProjectFullPath)`)) + <_PurviewProjectText>$([System.Text.RegularExpressions.Regex]::Replace($(_PurviewProjectRawText), `(?s)<!--.*?-->`, ``)) true false false true false true <_PurviewProjectDeclaresTargetFramework - Condition="$([System.Text.RegularExpressions.Regex]::IsMatch($([System.Text.RegularExpressions.Regex]::Replace($([System.IO.File]::ReadAllText(`$(MSBuildProjectFullPath)`)), `(?s)<!--.*?-->`, ``)), `(?is)<TargetFrameworks?\s*>`))" + Condition="$([System.Text.RegularExpressions.Regex]::IsMatch($(_PurviewProjectText), `(?is)<TargetFrameworks?\s*>`))" >true <_PurviewProjectDeclaresRootNamespace - Condition="$([System.Text.RegularExpressions.Regex]::IsMatch($([System.Text.RegularExpressions.Regex]::Replace($([System.IO.File]::ReadAllText(`$(MSBuildProjectFullPath)`)), `(?s)<!--.*?-->`, ``)), `(?is)<RootNamespace\s*>\s*\S`))" + Condition="$([System.Text.RegularExpressions.Regex]::IsMatch($(_PurviewProjectText), `(?is)<RootNamespace\s*>\s*\S`))" >true - <_PurviewProjectTargetFrameworkSet>$([System.Text.RegularExpressions.Regex]::Match($([System.Text.RegularExpressions.Regex]::Replace($([System.IO.File]::ReadAllText(`$(MSBuildProjectFullPath)`)), `(?s)<!--.*?-->`, ``)), `(?is)<PurviewTargetFrameworkSet\s*>\s*(?<set>[^<]*?)\s*</PurviewTargetFrameworkSet\s*>`).Groups['set'].Value) + <_PurviewProjectTargetFrameworkSet>$([System.Text.RegularExpressions.Regex]::Match($(_PurviewProjectText), `(?is)<PurviewTargetFrameworkSet\s*>\s*(?<set>[^<]*?)\s*</PurviewTargetFrameworkSet\s*>`).Groups['set'].Value) <_PurviewProjectDeclaresTargetFrameworkSet Condition="'$(_PurviewProjectTargetFrameworkSet)' != ''" @@ -232,7 +241,7 @@ The leading `(?:^|\s|>)` appeared twice, which required two whitespace/`>` characters before the element; it is matched once now. --> - $([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText(`$(MSBuildProjectFullPath)`)), `(?s-i)(?:^|\s|>)<\s*(?:Project|Import)\s(?:[^>]*?)\s?Sdk\s*="(?<sdkproj>.*?)"`).Groups['sdkproj'].Value) + $([System.Text.RegularExpressions.Regex]::Match($(_PurviewProjectRawText), `(?s-i)(?:^|\s|>)<\s*(?:Project|Import)\s(?:[^>]*?)\s?Sdk\s*="(?<sdkproj>.*?)"`).Groups['sdkproj'].Value) true true true diff --git a/src/src/BuildSdk/Sdk/Sdk.targets b/src/src/BuildSdk/Sdk/Sdk.targets index cce3470..afd6905 100644 --- a/src/src/BuildSdk/Sdk/Sdk.targets +++ b/src/src/BuildSdk/Sdk/Sdk.targets @@ -1233,6 +1233,12 @@ + + @@ -1415,6 +1421,34 @@ return !Log.HasLoggedErrors; } + // The first non-empty line, trimmed. A marker on line one survives an editor that + // adds or strips a leading blank line. + string FirstLine(string path) + { + try + { + foreach (var line in File.ReadLines(path)) + { + if (!string.IsNullOrWhiteSpace(line)) + { + return line.Trim(); + } + } + } + catch (IOException) + { + // Unreadable means "not recognisably ours", which is the safe answer. + } + catch (UnauthorizedAccessException) + { + } + + return string.Empty; + } + + bool IsManagedByMarker(string path, string prefix) => + FirstLine(path).StartsWith(prefix, StringComparison.Ordinal); + var destinationFile = DestinationFile ?? string.Empty; if (string.IsNullOrEmpty(destinationFile)) { @@ -1424,7 +1458,36 @@ if (File.Exists(destinationFile)) { - if (!OverwriteExisting) + // A destination carrying the SDK's own version marker is a file the SDK wrote, + // so it is refreshed when the version moves on. A destination without the + // marker belongs to the developer and is never touched, whatever the version + // says - which is what makes deleting the marker line a supported way to take + // ownership of the file. + var managedUpdate = + !OverwriteExisting + && !string.IsNullOrEmpty(VersionMarkerPrefix) + && !string.IsNullOrEmpty(SourceFile) + && File.Exists(SourceFile) + && IsManagedByMarker(destinationFile, VersionMarkerPrefix) + && !string.Equals( + FirstLine(destinationFile), + FirstLine(SourceFile), + StringComparison.Ordinal + ); + + if (managedUpdate) + { + Log.LogMessage( + MessageImportance.Normal, + "Updating '{0}' from '{1}': the SDK-managed version marker changed from '{2}' to '{3}'.", + destinationFile, + SourceFile, + FirstLine(destinationFile), + FirstLine(SourceFile) + ); + } + + if (!OverwriteExisting && !managedUpdate) { // Bootstraps are user-owned: an existing file is never touched, which // turns a lost race between parallel projects into a no-op. @@ -1601,6 +1664,13 @@ > <_DirectoryBuildPropsDirectory>$([System.IO.Path]::GetDirectoryName('$(DirectoryBuildPropsPath)')) + + <_PurviewEditorConfigVersionMarkerPrefix Condition="'$(PurviewEditorConfigAutoUpdate)' != 'false'" + >$(PurviewEditorConfigVersionMarker) @@ -1651,15 +1721,104 @@ Condition="'$(RepositoryEditorConfigFilePath)' == '' AND '$(_EffectiveRepositoryEditorConfigFilePath)' != ''" >$(_EffectiveRepositoryEditorConfigFilePath) + + + <_PurviewEditorConfigDestinationExists Condition="'$(_EffectiveRepositoryEditorConfigFilePath)' != ''" + >$([System.IO.File]::Exists('$(_EffectiveRepositoryEditorConfigFilePath)')) + <_PurviewEditorConfigDestinationFirstLine + Condition="'$(_PurviewEditorConfigDestinationExists)' == 'true' AND '$(_PurviewEditorConfigVersionMarkerPrefix)' != ''" + >$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(_EffectiveRepositoryEditorConfigFilePath)')), `^[^\r\n]*`).Value) + <_PurviewEditorConfigSourceFirstLine + Condition="'$(_PurviewEditorConfigDestinationExists)' == 'true' AND '$(_PurviewEditorConfigVersionMarkerPrefix)' != ''" + >$([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText('$(EditorConfigFilePath)')), `^[^\r\n]*`).Value) + <_PurviewEditorConfigIsManaged + Condition=" + '$(_PurviewEditorConfigDestinationFirstLine)' != '' + AND $(_PurviewEditorConfigDestinationFirstLine.StartsWith('$(_PurviewEditorConfigVersionMarkerPrefix)'))" + >true + + + <_PurviewEditorConfigNeedsSync Condition="'$(_PurviewEditorConfigDestinationExists)' != 'true'" + >true + + <_PurviewEditorConfigNeedsSync + Condition="'$(PurviewRepoBootstrapMode)' == 'Always' OR '$(PurviewRepoBootstrapMode)' == 'WarnOnDrift'" + >true + + <_PurviewEditorConfigNeedsSync + Condition=" + '$(_PurviewEditorConfigIsManaged)' == 'true' + AND '$(_PurviewEditorConfigDestinationFirstLine)' != '$(_PurviewEditorConfigSourceFirstLine)'" + >true + + + <_PurviewEditorConfigIsUnadopted + Condition=" + '$(_PurviewEditorConfigDestinationExists)' == 'true' + AND '$(_PurviewEditorConfigVersionMarkerPrefix)' != '' + AND '$(_PurviewEditorConfigIsManaged)' != 'true' + AND '$(PurviewRepoBootstrapMode)' == 'IfMissing' + AND '$(PurviewEditorConfigAdoptionWarning)' == 'true' + AND '$(DesignTimeBuild)' != 'true' + AND ('$(PurviewEditorConfigAdoptionWarningInCI)' == 'true' + OR ('$(ContinuousIntegrationBuild)' != 'true' AND '$(TF_BUILD)' != 'true' + AND '$(GITHUB_ACTIONS)' != 'true' AND '$(CI)' != 'true'))" + >true + + + $([MSBuild]::NormalizePath('$(_RepositoryEditorConfigDirectory)', '.purview', 'editorconfig-adoption.cache')) + + <_PurviewEditorConfigAdoptionStampValue Condition="Exists('$(PurviewEditorConfigAdoptionStampPath)')" + >$([System.String]::Copy($([System.IO.File]::ReadAllText('$(PurviewEditorConfigAdoptionStampPath)'))).Trim()) + <_PurviewEditorConfigReportAdoption + Condition="'$(_PurviewEditorConfigAdoptionStampValue)' != '$(_PurviewEditorConfigSourceFirstLine)'" + >true + + + @@ -2321,15 +2480,39 @@ This target is what keeps a malformed skill from reaching the CLI: it runs at pack time in the repository that owns the skills, so the failure lands on the author rather than silently producing a skill the - CLI skips. + CLI skips. The folder it validates is the one every other skill target + resolves, because validating a different repository passes silently. ===================================================================== --> - + + + <_PurviewSkillsValidationRoot Condition="'$(_PurviewSkillsRepositoryRoot)' != ''" + >$([MSBuild]::NormalizeDirectory('$(_PurviewSkillsRepositoryRoot)', 'skills')) + + <_PurviewSkillsValidationRoot Condition="'$(_PurviewSkillsValidationRoot)' == ''" + >$(PurviewSkillsSourceFolder) + + + @@ -2746,7 +2932,7 @@ IgnoreStandardErrorWarningFormat="true" StandardErrorImportance="low" StandardOutputImportance="low" - Condition="'$(RepositoryGlobalJsonFilePath)' == '' AND '$(_DirectoryBuildPropsDirectory)' != ''" + Condition="'$(PurviewRepositoryRootStrategy)' == 'Git' AND '$(RepositoryGlobalJsonFilePath)' == '' AND '$(_DirectoryBuildPropsDirectory)' != ''" > @@ -2798,6 +2984,20 @@ <_RepositoryGlobalJsonLines Include=" }" /> <_RepositoryGlobalJsonLines Include="}" /> + + + <_PurviewGlobalJsonDestinationExists Condition="'$(_EffectiveRepositoryGlobalJsonFilePath)' != ''" + >$([System.IO.File]::Exists('$(_EffectiveRepositoryGlobalJsonFilePath)')) + <_PurviewGlobalJsonNeedsSync Condition="'$(_PurviewGlobalJsonDestinationExists)' != 'true'" + >true + <_PurviewGlobalJsonNeedsSync + Condition="'$(PurviewRepoBootstrapMode)' == 'Always' OR '$(PurviewRepoBootstrapMode)' == 'WarnOnDrift'" + >true + diff --git a/src/src/CodeFixers/CodeFixers.csproj b/src/src/CodeFixers/CodeFixers.csproj index b780ebf..cd0979f 100644 --- a/src/src/CodeFixers/CodeFixers.csproj +++ b/src/src/CodeFixers/CodeFixers.csproj @@ -12,6 +12,16 @@ + + + + + all diff --git a/src/src/CodeFixers/ExtensionsNamespace/SplitExtensionsClassCodeRefactoringProvider.cs b/src/src/CodeFixers/ExtensionsNamespace/SplitExtensionsClassCodeRefactoringProvider.cs index 9c81a21..8a16e2d 100644 --- a/src/src/CodeFixers/ExtensionsNamespace/SplitExtensionsClassCodeRefactoringProvider.cs +++ b/src/src/CodeFixers/ExtensionsNamespace/SplitExtensionsClassCodeRefactoringProvider.cs @@ -6,8 +6,8 @@ using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.FindSymbols; +using Microsoft.CodeAnalysis.Formatting; using Microsoft.CodeAnalysis.Rename; -using Microsoft.CodeAnalysis.Text; namespace Purview.BuildSdk.CodeFixers.ExtensionsNamespace; @@ -192,7 +192,17 @@ CancellationToken cancellationToken var @namespace = receiver.ContainingNamespace.ToDisplayString(); var typeName = StripInterfacePrefix(receiver.Name) + "Extensions"; var folderPath = ComputeConventionalFolders(@namespace); - var methodDeclarations = methods.Select(GetMethodSyntax).ToImmutableArray(); + // A method with no resolvable declaration is skipped rather than failing the refactoring. + var methodDeclarations = methods + .Select(GetMethodSyntax) + .Where(static declaration => declaration is not null) + .Select(static declaration => declaration!) + .ToImmutableArray(); + + if (methodDeclarations.Length == 0) + { + continue; + } var existingType = compilation?.GetTypeByMetadataName( @namespace.Length == 0 ? typeName : @namespace + "." + typeName @@ -263,11 +273,13 @@ CancellationToken cancellationToken classDeclaration ); - newSolution = newSolution.AddDocument( - DocumentId.CreateNewId(document.Project.Id), + newSolution = await AddFormattedDocumentAsync( + newSolution, + document.Project.Id, typeName + ".cs", - SourceText.From(newCompilationUnit.ToFullString()), - folderPath + newCompilationUnit, + folderPath, + cancellationToken ); } @@ -383,11 +395,13 @@ CancellationToken cancellationToken renamedClass ); - var newSolution = renamedSolution.AddDocument( - DocumentId.CreateNewId(document.Project.Id), + var newSolution = await AddFormattedDocumentAsync( + renamedSolution, + document.Project.Id, newTypeName + ".cs", - SourceText.From(newCompilationUnit.ToFullString()), - folderPath + newCompilationUnit, + folderPath, + cancellationToken ); newSolution = RemoveOriginalClassOrDocument(newSolution, document.Id, renamedCompilationUnit, renamedClass); @@ -683,7 +697,44 @@ ClassDeclarationSyntax classDeclaration .WithMembers(SyntaxFactory.SingletonList(classDeclaration)) ); - return newCompilationUnit.NormalizeWhitespace(); + // Annotated rather than NormalizeWhitespace()'d. NormalizeWhitespace imposes Roslyn's own defaults - + // four spaces and Allman braces - which ignores the .editorconfig this SDK ships and installs into + // every consuming repository, so a brand-new file would fail that repository's own formatting check + // the moment it was created. The annotation marks the generated syntax for AddFormattedDocumentAsync + // to format against the document's real options. + return newCompilationUnit.WithAdditionalAnnotations(Formatter.Annotation); + } + + /// + /// Adds a generated compilation unit as a new document and formats it against that document's own + /// options, so the result honours the consuming repository's .editorconfig. + /// + /// + /// The document is added as a syntax root rather than as text. Adding it as text would discard + /// the formatter annotations with the syntax tree that carried them, which is what forces the + /// alternative of baking Roslyn's default formatting in with NormalizeWhitespace. + /// + static async Task AddFormattedDocumentAsync( + Solution solution, + ProjectId projectId, + string fileName, + CompilationUnitSyntax compilationUnit, + IEnumerable folders, + CancellationToken cancellationToken + ) + { + var documentId = DocumentId.CreateNewId(projectId); + var withDocument = solution.AddDocument(documentId, fileName, compilationUnit, folders); + + var added = withDocument.GetDocument(documentId); + if (added is null) + { + return withDocument; + } + + var formatted = await Formatter.FormatAsync(added, Formatter.Annotation, null, cancellationToken); + + return formatted.Project.Solution; } static string[] ComputeConventionalFolders(string @namespace) => @@ -712,15 +763,17 @@ ClassDeclarationSyntax originalClass return solution.WithDocumentSyntaxRoot(documentId, updatedRoot, PreservationMode.PreserveIdentity); } - static MethodDeclarationSyntax GetMethodSyntax(IMethodSymbol method) - { - var reference = method.DeclaringSyntaxReferences.FirstOrDefault(); - if (reference?.GetSyntax() is MethodDeclarationSyntax syntax) - return syntax; - - // If the method is declared in source but we can't get its syntax, something is wrong. - throw new InvalidOperationException($"Unable to resolve the declaration of '{method.Name}'."); - } + /// + /// Resolves a method symbol back to its declaration, or when it has none. + /// + /// + /// A symbol without a source declaration - one from metadata, or from a partial compilation the IDE is + /// still building - is a reason to leave that method where it is, not a reason to fail. Throwing here + /// would take down the light-bulb for the whole refactoring with an exception the developer can neither + /// read nor act on; the caller skips the method instead. + /// + internal static MethodDeclarationSyntax? GetMethodSyntax(IMethodSymbol method) => + method.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as MethodDeclarationSyntax; static string StripInterfacePrefix(string name) => name.Length > 1 && name[0] == 'I' && char.IsUpper(name[1]) ? name.Substring(1) : name; diff --git a/src/src/SkillsTesting/PurviewSkillTests.cs b/src/src/SkillsTesting/PurviewSkillTests.cs index 1fe33ba..29804f3 100644 --- a/src/src/SkillsTesting/PurviewSkillTests.cs +++ b/src/src/SkillsTesting/PurviewSkillTests.cs @@ -39,8 +39,6 @@ namespace Purview.Skills.Testing; )] public abstract class PurviewSkillTests { - SkillPolicyEvaluator? _evaluator; - /// The repository root. Defaults to the SDK's repository-root marker walk. protected virtual string? RepositoryRoot => null; @@ -53,7 +51,7 @@ public abstract class PurviewSkillTests /// The evaluator, created once per test class instance. protected SkillPolicyEvaluator Evaluator => - _evaluator ??= SkillPolicyEvaluator.ForRepository(RepositoryRoot, SkillsFolderName); + field ??= SkillPolicyEvaluator.ForRepository(RepositoryRoot, SkillsFolderName); /// Reports whether a rule should be skipped for this repository. /// The rule identifier, from . diff --git a/src/src/SkillsTesting/SkillManifest.cs b/src/src/SkillsTesting/SkillManifest.cs index af61027..7de85e3 100644 --- a/src/src/SkillsTesting/SkillManifest.cs +++ b/src/src/SkillsTesting/SkillManifest.cs @@ -37,7 +37,7 @@ public sealed record SkillManifest /// The optional metadata mapping. public IReadOnlyDictionary Metadata { get; init; } = - new Dictionary(System.StringComparer.Ordinal); + new Dictionary(StringComparer.Ordinal); /// Top-level frontmatter keys the specification does not define. /// diff --git a/src/src/SkillsTesting/SkillManifestReader.cs b/src/src/SkillsTesting/SkillManifestReader.cs index 177c78c..d189300 100644 --- a/src/src/SkillsTesting/SkillManifestReader.cs +++ b/src/src/SkillsTesting/SkillManifestReader.cs @@ -45,6 +45,7 @@ public static SkillManifest Read(string skillDirectory) }; } + // The file exists, so read it and parse it. return Parse(folderName, skillDirectory, skillFilePath, File.ReadAllText(skillFilePath)); } diff --git a/src/src/SkillsTesting/SkillPolicyEvaluator.cs b/src/src/SkillsTesting/SkillPolicyEvaluator.cs index 26f67dc..b1e9c56 100644 --- a/src/src/SkillsTesting/SkillPolicyEvaluator.cs +++ b/src/src/SkillsTesting/SkillPolicyEvaluator.cs @@ -83,6 +83,7 @@ public IReadOnlyList Discovery() if (!Catalogue.FolderExists || Catalogue.Skills.Count > 0) return []; + // The folder exists but holds no skills, so report it as a violation rather than letting every return [ new SkillViolation( diff --git a/src/src/SkillsTesting/SkillsTesting.csproj b/src/src/SkillsTesting/SkillsTesting.csproj index 0570024..719ab08 100644 --- a/src/src/SkillsTesting/SkillsTesting.csproj +++ b/src/src/SkillsTesting/SkillsTesting.csproj @@ -1,4 +1,12 @@ + + + true + 1.0.7 + + Purview.Skills.Testing Purview Skills Testing diff --git a/src/tests/Analyzers.IntegrationTests/AnalyzerTestInfrastructure.cs b/src/tests/Analyzers.IntegrationTests/AnalyzerTestInfrastructure.cs index 5bc4180..f5f06f3 100644 --- a/src/tests/Analyzers.IntegrationTests/AnalyzerTestInfrastructure.cs +++ b/src/tests/Analyzers.IntegrationTests/AnalyzerTestInfrastructure.cs @@ -17,9 +17,22 @@ static class AnalyzerTestInfrastructure public static ImmutableArray BuildBclReferences() => [.. TrustedAssemblies.Select(p => MetadataReference.CreateFromFile(p))]; - public static CSharpCompilation CreateCompilation(string source, string filePath) + /// The source code to parse. + /// The file path to associate with the syntax tree. + /// + /// The language version to parse with. Defaults to the parser's own default; pass + /// for source that uses a preview feature, such as a + /// union declaration. + /// + public static CSharpCompilation CreateCompilation( + string source, + string filePath, + LanguageVersion? languageVersion = null + ) { - var syntaxTree = CSharpSyntaxTree.ParseText(source, path: NormalizeFakePath(filePath)); + var parseOptions = languageVersion is { } version ? new CSharpParseOptions(version) : null; + var syntaxTree = CSharpSyntaxTree.ParseText(source, parseOptions, path: NormalizeFakePath(filePath)); + return CSharpCompilation.Create( "TestAssembly", [syntaxTree], diff --git a/src/tests/Analyzers.IntegrationTests/ExtensionBlocks/ExtensionBlockNestingSuppressorTests.cs b/src/tests/Analyzers.IntegrationTests/ExtensionBlocks/ExtensionBlockNestingSuppressorTests.cs new file mode 100644 index 0000000..cac501f --- /dev/null +++ b/src/tests/Analyzers.IntegrationTests/ExtensionBlocks/ExtensionBlockNestingSuppressorTests.cs @@ -0,0 +1,215 @@ +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.BuildSdk.Analyzers.ExtensionBlocks; + +/// +/// Integration tests for behaviour. +/// +/// +/// CA1034 is reported by a stand-in analyzer rather than by the real CodeQuality rule, matching +/// UnionEqualitySuppressorTests. A suppressor only ever sees a diagnostic id and a location, so the +/// rule's own implementation is not what is under test — which declarations the suppression lands on is. The +/// stand-in reports on every externally visible nested type at that type's own source location, which is +/// what the real rule does and why an extension block is caught by it at all: the block is compiled into a +/// nested type, and the report lands on the extension keyword naming nothing. +/// +[Category("Integration")] +sealed class ExtensionBlockNestingSuppressorTests +{ + const string ExtensionBlockSource = """ + namespace Sample + { + public static class StringExtensions + { + extension(string value) + { + public bool IsBlank => string.IsNullOrWhiteSpace(value); + } + + public class Nested + { + public int Value; + } + } + } + """; + + static async Task AnalyzeAsync(string source, CancellationToken cancellationToken) + { + var compilation = AnalyzerTestInfrastructure.CreateCompilation( + source, + @"C:\FakeProject\Extensions.cs", + LanguageVersion.Preview + ); + + var analyzers = ImmutableArray.Create( + new FakeCa1034Analyzer(), + new ExtensionBlockNestingSuppressor() + ); + + // reportSuppressedDiagnostics keeps a suppressed diagnostic in the results, carrying IsSuppressed. + // Without it a suppressed diagnostic is simply filtered out, and "suppressed" would be + // indistinguishable from "the stand-in analyzer never reported it" - so a broken stand-in would make + // every suppression test pass for the wrong reason. + CompilationWithAnalyzersOptions analysisOptions = new( + AnalyzerTestInfrastructure.CreateAnalyzerOptions(), + onAnalyzerException: null, + concurrentAnalysis: false, + logAnalyzerExecutionTime: false, + reportSuppressedDiagnostics: true + ); + + var diagnostics = await compilation + .WithAnalyzers(analyzers, analysisOptions) + .GetAllDiagnosticsAsync(cancellationToken); + + return [.. diagnostics.Where(diagnostic => diagnostic.Id == "CA1034")]; + } + + static async Task AssertSuppressedAsync(Diagnostic[] diagnostics, string target, bool expected) + { + var diagnostic = diagnostics.SingleOrDefault(d => TargetName(d) == target); + + await Assert.That(diagnostic).IsNotNull(); + await Assert.That(diagnostic!.IsSuppressed).IsEqualTo(expected); + } + + [Test] + public async Task Suppressor_GivenAnExtensionBlock_SuppressesCa1034(CancellationToken cancellationToken) + { + var diagnostics = await AnalyzeAsync(ExtensionBlockSource, cancellationToken); + + await AssertSuppressedAsync(diagnostics, ExtensionTarget, expected: true); + } + + /// + /// An ordinary nested type keeps the warning, even when it shares a static class with an extension block. + /// This is the assertion that stops the suppression widening into "every type in an extension holder". + /// + [Test] + public async Task Suppressor_GivenAnOrdinaryTypeNestedBesideAnExtensionBlock_DoesNotSuppressCa1034( + CancellationToken cancellationToken + ) + { + var diagnostics = await AnalyzeAsync(ExtensionBlockSource, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "Nested", expected: false); + } + + /// + /// A type nested inside an extension block's own body is still an ordinary nested type. + /// + [Test] + public async Task Suppressor_GivenATypeNestedInAnExtensionBlock_SuppressesOnlyTheBlock( + CancellationToken cancellationToken + ) + { + const string source = """ + namespace Sample + { + public static class StringExtensions + { + extension(string value) + { + public bool IsBlank => string.IsNullOrWhiteSpace(value); + + public class Inner + { + public int Value; + } + } + } + } + """; + + var diagnostics = await AnalyzeAsync(source, cancellationToken); + + await AssertSuppressedAsync(diagnostics, ExtensionTarget, expected: true); + await AssertSuppressedAsync(diagnostics, "Inner", expected: false); + } + + [Test] + public async Task Suppressor_GivenAnOrdinaryNestedType_DoesNotSuppressCa1034(CancellationToken cancellationToken) + { + const string source = """ + namespace Sample + { + public class Outer + { + public class Nested + { + public int Value; + } + } + } + """; + + var diagnostics = await AnalyzeAsync(source, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "Nested", expected: false); + } + + /// + /// The name the stand-in reports for the extension block's generated type, which has no source name. + /// + const string ExtensionTarget = ""; + + static string TargetName(Diagnostic diagnostic) => diagnostic.Properties["TypeName"]!; + + /// + /// Reports CA1034 on every externally visible nested type, the way the real rule does, carrying the type + /// name so a test can tell the reports apart. + /// + [SuppressMessage( + "MicrosoftCodeAnalysisDesign", + "RS1029:Do not use reserved diagnostic IDs", + Justification = "The id is deliberately the real CA1034, because that is the id the suppressor under test declares as suppressible. This analyzer is a test stand-in and never ships." + )] + [DiagnosticAnalyzer(LanguageNames.CSharp)] + sealed class FakeCa1034Analyzer : DiagnosticAnalyzer + { + static readonly DiagnosticDescriptor Rule = new( + "CA1034", + "Nested types should not be visible", + "Do not nest type {0}", + "Design", + DiagnosticSeverity.Warning, + isEnabledByDefault: true + ); + + public override ImmutableArray SupportedDiagnostics => [Rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSymbolAction(AnalyzeType, SymbolKind.NamedType); + } + + static void AnalyzeType(SymbolAnalysisContext context) + { + if ( + context.Symbol is not INamedTypeSymbol { ContainingType: not null } type + || type.Locations.FirstOrDefault(location => location.IsInSource) is not { } location + ) + { + return; + } + + var name = string.IsNullOrEmpty(type.Name) ? ExtensionTarget : type.Name; + + context.ReportDiagnostic( + Diagnostic.Create( + Rule, + location, + ImmutableDictionary.Empty.Add("TypeName", name), + name + ) + ); + } + } +} diff --git a/src/tests/Analyzers.IntegrationTests/Extensions/SplitExtensionsClassMethodSyntaxTests.cs b/src/tests/Analyzers.IntegrationTests/Extensions/SplitExtensionsClassMethodSyntaxTests.cs new file mode 100644 index 0000000..10c9afe --- /dev/null +++ b/src/tests/Analyzers.IntegrationTests/Extensions/SplitExtensionsClassMethodSyntaxTests.cs @@ -0,0 +1,99 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Purview.BuildSdk.CodeFixers.ExtensionsNamespace; + +namespace Purview.BuildSdk.Analyzers.IntegrationTests.Extensions; + +/// +/// Covers , which resolves a method +/// symbol back to its declaration. +/// +/// +/// It used to throw when a symbol had no source declaration. A refactoring that throws takes down the +/// light-bulb for the whole action with an exception the developer can neither read nor act on, so the +/// unresolvable method is skipped instead. The null path is defensive — every method the refactoring groups +/// comes from the source document it was invoked on — which is exactly why it is worth pinning directly: +/// there is no input to the public entry point that reaches it. +/// +[Category("Integration")] +sealed class SplitExtensionsClassMethodSyntaxTests +{ + const string Source = """ + using System.Collections.Generic; + using System.Linq; + + namespace Sample + { + public static class ThingExtensions + { + public static string Describe(this string value) => value; + } + } + """; + + static CSharpCompilation CreateCompilation() => + AnalyzerTestInfrastructure.CreateCompilation(Source, @"C:\FakeProject\ThingExtensions.cs"); + + [Test] + public async Task GetMethodSyntax_GivenASourceDeclaredMethod_ReturnsItsDeclaration() + { + var compilation = CreateCompilation(); + var extensions = compilation.GetTypeByMetadataName("Sample.ThingExtensions"); + + await Assert.That(extensions).IsNotNull(); + + var method = extensions!.GetMembers("Describe").OfType().Single(); + + var syntax = SplitExtensionsClassCodeRefactoringProvider.GetMethodSyntax(method); + + await Assert.That(syntax).IsNotNull(); + await Assert.That(syntax!.Identifier.Text).IsEqualTo("Describe"); + } + + /// + /// A method from metadata has no at all, which is the + /// shape that used to throw. + /// + [Test] + public async Task GetMethodSyntax_GivenAMetadataMethod_ReturnsNull() + { + var compilation = CreateCompilation(); + var enumerable = compilation.GetTypeByMetadataName("System.Linq.Enumerable"); + + await Assert.That(enumerable).IsNotNull(); + + var method = enumerable!.GetMembers("Select").OfType().First(); + + await Assert.That(method.DeclaringSyntaxReferences).IsEmpty(); + await Assert.That(SplitExtensionsClassCodeRefactoringProvider.GetMethodSyntax(method)).IsNull(); + } + + /// + /// A symbol whose declaration is not a method declaration - a primary constructor's synthesised members, + /// a record's generated equality members - must also come back as null rather than failing a cast. + /// + [Test] + public async Task GetMethodSyntax_GivenASymbolWhoseDeclarationIsNotAMethod_ReturnsNull() + { + const string recordSource = """ + namespace Sample + { + public record Thing(int Id); + } + """; + + var compilation = AnalyzerTestInfrastructure.CreateCompilation(recordSource, @"C:\FakeProject\Thing.cs"); + + var thing = compilation.GetTypeByMetadataName("Sample.Thing"); + + await Assert.That(thing).IsNotNull(); + + // The primary constructor's declaration is the record declaration, not a MethodDeclarationSyntax. + var constructor = thing! + .GetMembers(".ctor") + .OfType() + .First(method => method.Parameters.Length == 1); + + await Assert.That(SplitExtensionsClassCodeRefactoringProvider.GetMethodSyntax(constructor)).IsNull(); + } +} diff --git a/src/tests/Analyzers.IntegrationTests/Unions/UnionEqualitySuppressorTests.cs b/src/tests/Analyzers.IntegrationTests/Unions/UnionEqualitySuppressorTests.cs new file mode 100644 index 0000000..1c05683 --- /dev/null +++ b/src/tests/Analyzers.IntegrationTests/Unions/UnionEqualitySuppressorTests.cs @@ -0,0 +1,239 @@ +using System.Collections.Immutable; +using System.Diagnostics.CodeAnalysis; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Purview.BuildSdk.Analyzers.Unions; + +/// +/// Integration tests for behaviour. +/// +/// +/// +/// CA1815 is reported by a stand-in analyzer rather than by the real CodeQuality rule, matching +/// ExtensionsNamespaceSuppressorTests. A suppressor only ever sees a diagnostic id and a location, so +/// the rule's own implementation is not what is under test — which declarations the suppression lands on is. +/// The stand-in reports on every struct, which is what the real rule does and which is why a union is caught +/// by it at all. +/// +/// +/// Every source here declares UnionAttribute and IUnion itself. Unions are a preview language +/// feature whose required runtime types are not in the net10.0 framework this test host references, and +/// without IUnion the union declaration fails to bind with CS0518 and no symbol is produced. +/// +/// +[Category("Integration")] +sealed class UnionEqualitySuppressorTests +{ + /// + /// The runtime types a union declaration binds against, declared in source because the targeted framework + /// does not supply them yet. The compiler embeds its own copies in exactly the same way. + /// + const string UnionRuntimeDeclarations = """ + namespace System.Runtime.CompilerServices + { + [global::System.AttributeUsage(global::System.AttributeTargets.Struct | global::System.AttributeTargets.Class)] + public sealed class UnionAttribute : global::System.Attribute { } + + public interface IUnion { object? Value { get; } } + } + """; + + static async Task AnalyzeAsync(string source, CancellationToken cancellationToken) + { + var compilation = AnalyzerTestInfrastructure.CreateCompilation( + source, + @"C:\FakeProject\Errors.cs", + LanguageVersion.Preview + ); + + var analyzers = ImmutableArray.Create( + new FakeCa1815Analyzer(), + new UnionEqualitySuppressor() + ); + + // reportSuppressedDiagnostics keeps a suppressed diagnostic in the results, carrying IsSuppressed. + // Without it a suppressed diagnostic is simply filtered out, and "suppressed" would be + // indistinguishable from "the stand-in analyzer never reported it" - so a broken stand-in would make + // every suppression test pass for the wrong reason. + CompilationWithAnalyzersOptions analysisOptions = new( + AnalyzerTestInfrastructure.CreateAnalyzerOptions(), + onAnalyzerException: null, + concurrentAnalysis: false, + logAnalyzerExecutionTime: false, + reportSuppressedDiagnostics: true + ); + + var diagnostics = await compilation + .WithAnalyzers(analyzers, analysisOptions) + .GetAllDiagnosticsAsync(cancellationToken); + + return [.. diagnostics.Where(diagnostic => diagnostic.Id == "CA1815")]; + } + + static async Task AssertSuppressedAsync(Diagnostic[] diagnostics, string typeName, bool expected) + { + var diagnostic = diagnostics.SingleOrDefault(d => TargetName(d) == typeName); + + await Assert.That(diagnostic).IsNotNull(); + await Assert.That(diagnostic!.IsSuppressed).IsEqualTo(expected); + } + + const string KeywordUnionSource = $$""" + {{UnionRuntimeDeclarations}} + + namespace Sample + { + public readonly record struct NotFound(int Id); + + public readonly record struct Disabled(int Id); + + public readonly union TenantError(NotFound, Disabled); + } + """; + + [Test] + public async Task Suppressor_GivenAUnionDeclaredWithTheKeyword_SuppressesCa1815(CancellationToken cancellationToken) + { + var diagnostics = await AnalyzeAsync(KeywordUnionSource, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "TenantError", expected: true); + } + + /// + /// The case types of a union are ordinary value types and keep the warning. This is the assertion that + /// stops the suppression widening into "every struct near a union". + /// + [Test] + [Arguments("NotFound")] + [Arguments("Disabled")] + public async Task Suppressor_GivenAUnionsCaseType_DoesNotSuppressCa1815( + string caseTypeName, + CancellationToken cancellationToken + ) + { + var diagnostics = await AnalyzeAsync(KeywordUnionSource, cancellationToken); + + await AssertSuppressedAsync(diagnostics, caseTypeName, expected: false); + } + + [Test] + public async Task Suppressor_GivenAnOrdinaryStruct_DoesNotSuppressCa1815(CancellationToken cancellationToken) + { + const string source = $$""" + {{UnionRuntimeDeclarations}} + + namespace Sample + { + public struct Money + { + public decimal Amount; + } + } + """; + + var diagnostics = await AnalyzeAsync(source, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "Money", expected: false); + } + + [Test] + public async Task Suppressor_GivenATypeCarryingUnionAttribute_SuppressesCa1815(CancellationToken cancellationToken) + { + const string source = $$""" + {{UnionRuntimeDeclarations}} + + namespace Sample + { + [global::System.Runtime.CompilerServices.Union] + public struct AttributedUnion + { + public int Tag; + } + } + """; + + var diagnostics = await AnalyzeAsync(source, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "AttributedUnion", expected: true); + } + + /// + /// The innermost declaration wins, so a union does not suppress the warning for a struct nested inside it. + /// + [Test] + public async Task Suppressor_GivenAStructNestedInAUnion_SuppressesOnlyTheUnion(CancellationToken cancellationToken) + { + const string source = $$""" + {{UnionRuntimeDeclarations}} + + namespace Sample + { + [global::System.Runtime.CompilerServices.Union] + public struct OuterUnion + { + public int Tag; + + public struct Nested + { + public int Value; + } + } + } + """; + + var diagnostics = await AnalyzeAsync(source, cancellationToken); + + await AssertSuppressedAsync(diagnostics, "OuterUnion", expected: true); + await AssertSuppressedAsync(diagnostics, "Nested", expected: false); + } + + static string TargetName(Diagnostic diagnostic) => diagnostic.Properties["TypeName"]!; + + /// + /// Reports CA1815 on every struct, the way the real rule does, carrying the type name so a test can tell + /// the reports apart. + /// + [SuppressMessage( + "MicrosoftCodeAnalysisDesign", + "RS1029:Do not use reserved diagnostic IDs", + Justification = "The id is deliberately the real CA1815, because that is the id the suppressor under test declares as suppressible. This analyzer is a test stand-in and never ships." + )] + [DiagnosticAnalyzer(LanguageNames.CSharp)] + sealed class FakeCa1815Analyzer : DiagnosticAnalyzer + { + static readonly DiagnosticDescriptor Rule = new( + "CA1815", + "Override equals and operator equals on value types", + "Override equals and operator equals on value types", + "Performance", + DiagnosticSeverity.Warning, + isEnabledByDefault: true + ); + + public override ImmutableArray SupportedDiagnostics => [Rule]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSymbolAction(AnalyzeType, SymbolKind.NamedType); + } + + static void AnalyzeType(SymbolAnalysisContext context) + { + if ( + context.Symbol is not INamedTypeSymbol { TypeKind: TypeKind.Struct } type + || type.Locations.FirstOrDefault(location => location.IsInSource) is not { } location + ) + { + return; + } + + context.ReportDiagnostic( + Diagnostic.Create(Rule, location, ImmutableDictionary.Empty.Add("TypeName", type.Name)) + ); + } + } +} diff --git a/src/tests/BuildSdk.IntegrationTests/DefaultsPropsTests.cs b/src/tests/BuildSdk.IntegrationTests/DefaultsPropsTests.cs index 5c199ae..3d296a3 100644 --- a/src/tests/BuildSdk.IntegrationTests/DefaultsPropsTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/DefaultsPropsTests.cs @@ -1,4 +1,4 @@ -using System.Diagnostics; +using Purview.BuildSdk.Infra; namespace Purview.BuildSdk; @@ -52,11 +52,10 @@ await File.WriteAllTextAsync( cancellationToken ); - var (exitCode, stdOut, stdErr) = await RunProcessAsync( - "dotnet", + var (exitCode, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( $"msbuild \"{testProjectPath}\" -nologo -t:NoOp", tempRoot, - cancellationToken + cancellationToken: cancellationToken ); var output = stdOut + stdErr; @@ -69,33 +68,4 @@ await File.WriteAllTextAsync( Directory.Delete(tempRoot, recursive: true); } } - - static async Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( - string fileName, - string arguments, - string workingDirectory, - CancellationToken cancellationToken - ) - { - using var process = new Process - { - StartInfo = new ProcessStartInfo - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - process.Start(); - var stdOutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stdErrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdOutTask, await stdErrTask); - } } diff --git a/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.Builder.cs b/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.Builder.cs index 2fa84d7..6475ce7 100644 --- a/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.Builder.cs +++ b/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.Builder.cs @@ -22,6 +22,7 @@ sealed class ProjectHarnessBuilder string? _preImportPropsRaw; string? _projectFileContent; string? _solutionDirectory; + bool _seedRepositoryBootstrapFiles = true; internal ProjectHarnessBuilder(string projectName) { @@ -128,6 +129,21 @@ public ProjectHarnessBuilder WithSolutionDirectory(string solutionDirectory) return this; } + /// + /// Leaves the throwaway solution without a .editorconfig or global.json, so the SDK's + /// repository bootstrap has work to do on the first build. + /// + /// + /// Only the tests that cover the bootstrap itself need this. Every other test seeds those files, + /// because a throwaway solution starts empty and would otherwise make the bootstrap compile and run its + /// inline task on every build — around 0.7s each, for a side effect the test is not looking at. + /// + public ProjectHarnessBuilder WithoutRepositoryBootstrapSeed() + { + _seedRepositoryBootstrapFiles = false; + return this; + } + public async Task BuildAsync(CancellationToken cancellationToken = default) { var workDir = string.IsNullOrWhiteSpace(_solutionDirectory) @@ -142,7 +158,13 @@ public async Task BuildAsync(CancellationToken cancellationToken ownsWorkDir ); - await harness.WriteBoilerplateAsync(_namespacePrefix, _projectName, _preImportPropsRaw, cancellationToken); + await harness.WriteBoilerplateAsync( + _namespacePrefix, + _projectName, + _preImportPropsRaw, + _seedRepositoryBootstrapFiles, + cancellationToken + ); if (!string.IsNullOrWhiteSpace(_projectFileContent)) { diff --git a/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.cs b/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.cs index 9e40d1d..ccf4a3f 100644 --- a/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.cs +++ b/src/tests/BuildSdk.IntegrationTests/Harness/ProjectHarness.cs @@ -1,7 +1,7 @@ using System.Collections.Immutable; -using System.Diagnostics; using System.Text.Json; using System.Xml.Linq; +using Purview.BuildSdk.Infra; namespace Purview.BuildSdk.Harness; @@ -53,6 +53,7 @@ public static async Task CreateAsync( string? extraItems = null, IDictionary? extraEnv = null, string? preImportProps = null, + bool seedRepositoryBootstrapFiles = true, CancellationToken cancellationToken = default ) { @@ -61,6 +62,9 @@ public static async Task CreateAsync( .WithTargetFramework(targetFramework) .WithNamespacePrefix(namespacePrefix); + if (!seedRepositoryBootstrapFiles) + builder.WithoutRepositoryBootstrapSeed(); + if (withDockerfile) builder.WithDockerfile(); @@ -96,10 +100,70 @@ public static async Task CreateWithContentAsync( .BuildAsync(cancellationToken); } + /// + /// The .editorconfig the SDK bootstraps from, resolved the same way Sdk.props resolves + /// EditorConfigFilePath when the SDK is imported from source: the packed copy if it exists, + /// otherwise the repository's own. + /// + static string SourceEditorConfigPath + { + get + { + var packaged = Path.Combine(SdkPaths.SdkDirectory, ".editorconfig"); + + return File.Exists(packaged) + ? packaged + : Path.GetFullPath(Path.Combine(SdkPaths.SdkDirectory, "..", "..", "..", "..", ".editorconfig")); + } + } + + /// + /// Seeds the files the SDK's repository bootstrap would otherwise create on the first build. + /// + /// + /// A throwaway solution starts empty, so without this every build gives the bootstrap real work and it + /// compiles and runs its RoslynCodeTaskFactory task — roughly 0.7s per build, for a side effect + /// almost no test is looking at. Seeding the destination with the exact source content makes the + /// bootstrap's own "nothing to do" check succeed, which is the same state a real repository is in on + /// every build after its first. + /// + /// The tests that cover the bootstrap opt out with + /// . + /// + async Task SeedRepositoryBootstrapFilesAsync(CancellationToken cancellationToken) + { + var editorConfigPath = Path.Combine(SolutionDirectory, ".editorconfig"); + if (!File.Exists(editorConfigPath) && File.Exists(SourceEditorConfigPath)) + { + File.Copy(SourceEditorConfigPath, editorConfigPath); + } + + var globalJsonPath = Path.Combine(SolutionDirectory, "global.json"); + if (!File.Exists(globalJsonPath)) + { + // Content is irrelevant to the bootstrap, which only checks that the file is there; this is + // the shape it would have written. No msbuild-sdks entry, because nothing in a throwaway + // project resolves an SDK by package and an unresolvable entry would fail the build. + await File.WriteAllTextAsync( + globalJsonPath, + /*lang=json,strict*/ + """ + { + "test": { + "runner": "Microsoft.Testing.Platform" + } + } + """, + cancellationToken + ); + } + } + internal async Task WriteBoilerplateAsync( string namespacePrefix, string projectName, string? preImportProps, + bool seedRepositoryBootstrapFiles, CancellationToken cancellationToken ) { @@ -162,6 +226,11 @@ await File.WriteAllTextAsync( cancellationToken ); } + + if (seedRepositoryBootstrapFiles) + { + await SeedRepositoryBootstrapFilesAsync(cancellationToken); + } } async Task CreateOrUpdateSolution(string solutionPath, string projectName, CancellationToken cancellationToken) @@ -530,80 +599,31 @@ CancellationToken cancellationToken SolutionDirectory ); + /// + /// Runs an arbitrary command through the harness, with the host CI environment stripped. Use this + /// rather than spawning a process directly: inheriting the runner's environment is how an + /// integration test starts asserting something different in CI than it does locally. + /// + public Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( + string fileName, + string arguments, + string? workingDirectory = null, + CancellationToken cancellationToken = default + ) => RunAsync(fileName, arguments, cancellationToken, workingDirectory); + async Task<(int Code, string StdOut, string StdErr)> RunAsync( string fileName, string arguments, CancellationToken cancellationToken, string? workingDirectory = null - ) - { - using Process process = new() - { - StartInfo = new() - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory ?? ProjectDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - if (_extraEnv.Count > 0) - { - foreach (var (key, value) in _extraEnv) - process.StartInfo.Environment[key] = value; - } - - IsolateFromHostEnvironment(process.StartInfo.Environment, _extraEnv); - - process.Start(); - - var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); - - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdoutTask, await stderrTask); - } - - /// - /// Throwaway consumer projects must not inherit the CI runner's environment. Env vars like - /// GITHUB_WORKSPACE (version detection), GITHUB_ACTIONS/TF_BUILD (CI - /// detection) and friends would change SDK behaviour and make tests environment-dependent. - /// They are removed unless the harness explicitly overrides them via . - /// - internal static void IsolateFromHostEnvironment( - IDictionary environment, - IReadOnlyDictionary overrides - ) - { - foreach (var name in CiEnvironmentVariables) - { - if (!overrides.ContainsKey(name)) - environment.Remove(name); - } - } - - static readonly string[] CiEnvironmentVariables = - [ - "CI", - "GITHUB_ACTIONS", - "GITHUB_WORKSPACE", - "GITHUB_SHA", - "GITHUB_REF", - "GITHUB_HEAD_REF", - "GITHUB_BASE_REF", - "TF_BUILD", - "BUILD_SOURCESDIRECTORY", - "BUILD_REPOSITORY_LOCALPATH", - "CI_PROJECT_DIR", - "RUNNER_OS", - "RUNNER_ARCH", - "RUNNER_TEMP", - ]; + ) => + await ProcessRunner.RunWithEnvironmentAsync( + fileName, + arguments, + workingDirectory ?? ProjectDirectory, + _extraEnv, + cancellationToken + ); public async Task GetPreprocessProjectAsync(CancellationToken cancellationToken) { diff --git a/src/tests/BuildSdk.IntegrationTests/Infra/ProcessRunner.cs b/src/tests/BuildSdk.IntegrationTests/Infra/ProcessRunner.cs new file mode 100644 index 0000000..f170bc9 --- /dev/null +++ b/src/tests/BuildSdk.IntegrationTests/Infra/ProcessRunner.cs @@ -0,0 +1,116 @@ +namespace Purview.BuildSdk.Infra; + +/// +/// The one place a test spawns a process. Every throwaway project is built, evaluated and packed +/// through here so that no test depends on the machine it happens to run on. +/// +/// +/// A throwaway consumer project must not inherit the CI runner's environment. Variables such as +/// GITHUB_WORKSPACE change where the SDK thinks the repository root is, and CI, +/// GITHUB_ACTIONS and TF_BUILD change which advice it suppresses, so an inheriting test +/// asserts something different in CI than it does locally. One test did exactly that and reported a +/// failure that could not be reproduced off the runner. +/// +/// They are removed unless the caller explicitly overrides them, which is how a test simulates a CI +/// environment on purpose. Keeping the list in one place is what stops the next test from quietly +/// needing its own copy. +/// +static class ProcessRunner +{ + /// + /// Runs with the host CI environment stripped, returning its exit + /// code, standard output and standard error. + /// + public static Task<(int Code, string StdOut, string StdErr)> RunAsync( + string fileName, + string arguments, + string workingDirectory, + CancellationToken cancellationToken = default + ) => RunWithEnvironmentAsync(fileName, arguments, workingDirectory, environment: null, cancellationToken); + + /// + /// Runs dotnet, which is what almost every call site wants. + /// + public static Task<(int Code, string StdOut, string StdErr)> RunDotNetAsync( + string arguments, + string workingDirectory, + CancellationToken cancellationToken = default + ) => RunAsync("dotnet", arguments, workingDirectory, cancellationToken); + + /// + /// As , but with also set on the child process. + /// The remaining CI variables are still stripped, so a test can opt back in to exactly the ones it + /// is exercising and no others. + /// + public static async Task<(int Code, string StdOut, string StdErr)> RunWithEnvironmentAsync( + string fileName, + string arguments, + string workingDirectory, + IReadOnlyDictionary? environment, + CancellationToken cancellationToken = default + ) + { + using var process = new System.Diagnostics.Process + { + StartInfo = new System.Diagnostics.ProcessStartInfo + { + FileName = fileName, + Arguments = arguments, + WorkingDirectory = workingDirectory, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }, + }; + + if (environment is not null) + { + foreach (var (key, value) in environment) + process.StartInfo.Environment[key] = value; + } + + IsolateFromHostEnvironment(process.StartInfo.Environment, environment); + + process.Start(); + var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); + var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); + await process.WaitForExitAsync(cancellationToken); + + return (process.ExitCode, await stdoutTask, await stderrTask); + } + + /// + /// Removes the CI runner's environment from a child process, unless the caller overrode the + /// variable on purpose. + /// + internal static void IsolateFromHostEnvironment( + IDictionary environment, + IReadOnlyDictionary? overrides + ) + { + foreach (var name in CiEnvironmentVariables) + { + if (overrides is null || !overrides.ContainsKey(name)) + environment.Remove(name); + } + } + + static readonly string[] CiEnvironmentVariables = + [ + "CI", + "GITHUB_ACTIONS", + "GITHUB_WORKSPACE", + "GITHUB_SHA", + "GITHUB_REF", + "GITHUB_HEAD_REF", + "GITHUB_BASE_REF", + "TF_BUILD", + "BUILD_SOURCESDIRECTORY", + "BUILD_REPOSITORY_LOCALPATH", + "CI_PROJECT_DIR", + "RUNNER_OS", + "RUNNER_ARCH", + "RUNNER_TEMP", + ]; +} diff --git a/src/tests/BuildSdk.IntegrationTests/PackableProjectDefaultsTests.cs b/src/tests/BuildSdk.IntegrationTests/PackableProjectDefaultsTests.cs index b2ddb75..f06cdc4 100644 --- a/src/tests/BuildSdk.IntegrationTests/PackableProjectDefaultsTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PackableProjectDefaultsTests.cs @@ -1,4 +1,3 @@ -using System.Diagnostics; using System.IO.Compression; using System.Xml.Linq; using Purview.BuildSdk.Harness; @@ -356,8 +355,7 @@ await File.WriteAllTextAsync( var feedDirectory = Path.Combine(sharedDir, "feed"); var solutionPath = Path.Combine(sharedDir, "TestingSolution.slnx"); - var (exitCode, stdOut, stdErr) = await RunProcessAsync( - "dotnet", + var (exitCode, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( $"pack \"{solutionPath}\" -c Release -o \"{feedDirectory}\"", sharedDir, cancellationToken @@ -416,8 +414,7 @@ await File.WriteAllTextAsync( var feedDirectory = Path.Combine(sharedDir, "feed"); var solutionPath = Path.Combine(sharedDir, "TestingSolution.slnx"); - var (exitCode, stdOut, stdErr) = await RunProcessAsync( - "dotnet", + var (exitCode, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( $"pack \"{solutionPath}\" -c Release -o \"{feedDirectory}\"", sharedDir, cancellationToken @@ -462,8 +459,7 @@ CancellationToken cancellationToken ) { var feedDirectory = Path.Combine(harness.SolutionDirectory, "feed"); - return await RunProcessAsync( - "dotnet", + return await ProcessRunner.RunDotNetAsync( $"pack \"{harness.ProjectFilePath}\" -c Release -o \"{feedDirectory}\"", harness.SolutionDirectory, cancellationToken @@ -479,38 +475,4 @@ static string ExtractPackVersion(string stdOut, string packageId) return match.Success ? match.Groups[1].Value : string.Empty; } - - static async Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( - string fileName, - string arguments, - string workingDirectory, - CancellationToken cancellationToken - ) - { - using var process = new Process - { - StartInfo = new ProcessStartInfo - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - ProjectHarness.IsolateFromHostEnvironment( - process.StartInfo.Environment, - new Dictionary(StringComparer.OrdinalIgnoreCase) - ); - - process.Start(); - var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdoutTask, await stderrTask); - } } diff --git a/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs b/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs new file mode 100644 index 0000000..ba12819 --- /dev/null +++ b/src/tests/BuildSdk.IntegrationTests/PackagedVersionFallbackTests.cs @@ -0,0 +1,211 @@ +using System.Text.Json; +using System.Text.RegularExpressions; +using Purview.BuildSdk.Infra; + +namespace Purview.BuildSdk; + +/// +/// Pins the version literals that are packed into the SDK, or shown to someone adopting it, against the +/// single source of truth in package.json. +/// +/// +/// These drift silently and are only noticed by a consumer. PurviewBuildSdkVersionForGlobalJson is +/// the one that bites: it is the version written into a bootstrapped global.json when the SDK +/// cannot detect its own version from the restored package path, so a stale fallback pins a brand-new +/// repository to a release that predates every fix in the SDK doing the bootstrapping. +/// +public sealed partial class PackagedVersionFallbackTests +{ + [GeneratedRegex( + @"]*>\s*(?[^<\s]+)\s*", + RegexOptions.Singleline + )] + private static partial Regex GlobalJsonVersionFallback(); + + [GeneratedRegex(@"""Purview\.BuildSdk""\s*:\s*""(?[^""]+)""")] + private static partial Regex BuildSdkPin(); + + [GeneratedRegex(@"^#\s*Purview\.BuildSdk\s+(?\S+)")] + private static partial Regex EditorConfigVersionMarker(); + + [GeneratedRegex( + @"]*>\s*(?.*?)", + RegexOptions.Singleline + )] + private static partial Regex EditorConfigVersionMarkerRegex(); + + static string RepositoryRoot + { + get + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null) + { + if (File.Exists(Path.Combine(directory.FullName, "AGENTS.md"))) + return directory.FullName; + + directory = directory.Parent; + } + + throw new InvalidOperationException( + $"Could not locate the repository root above '{AppContext.BaseDirectory}'." + ); + } + } + + static string PackageVersion() + { + using var document = JsonDocument.Parse(File.ReadAllText(Path.Combine(RepositoryRoot, "package.json"))); + + return document.RootElement.GetProperty("version").GetString() + ?? throw new InvalidOperationException("package.json has no 'version'."); + } + + [Test] + public async Task GlobalJsonVersionFallback_MatchesPackageJson(CancellationToken cancellationToken) + { + var defaults = await File.ReadAllTextAsync( + Path.Combine(RepositoryRoot, "src", "src", "BuildSdk", "Sdk", "Props", "Defaults.props"), + cancellationToken + ); + + var match = GlobalJsonVersionFallback().Match(defaults); + + await Assert.That(match.Success).IsTrue(); + await Assert.That(match.Groups["version"].Value).IsEqualTo(PackageVersion()); + } + + /// + /// The marker has to be the first line of the repository's .editorconfig, because that is where + /// 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. + /// + [Test] + public async Task EditorConfigVersionMarker_IsTheFirstLineAndWellFormed(CancellationToken cancellationToken) + { + var lines = await File.ReadAllLinesAsync(Path.Combine(RepositoryRoot, ".editorconfig"), cancellationToken); + + await Assert.That(lines).IsNotEmpty(); + + var match = EditorConfigVersionMarker().Match(lines[0]); + + await Assert.That(match.Success).IsTrue(); + await Assert.That(Version.TryParse(match.Groups["version"].Value, out _)).IsTrue(); + } + + /// + /// The marker the SDK matches on at build time has to be a prefix of the line it actually writes, or a + /// managed file is never recognised as managed and the update silently never happens. + /// + [Test] + public async Task EditorConfigVersionMarker_IsPrefixedByTheDefaultsPropsMarker(CancellationToken cancellationToken) + { + var defaults = await File.ReadAllTextAsync( + Path.Combine(RepositoryRoot, "src", "src", "BuildSdk", "Sdk", "Props", "Defaults.props"), + cancellationToken + ); + + var marker = EditorConfigVersionMarkerRegex() + .Match(defaults) + .Groups["marker"] + .Value.Replace(" ", " ", StringComparison.Ordinal); + + await Assert.That(marker).IsNotEmpty(); + + var firstLine = ( + await File.ReadAllLinesAsync(Path.Combine(RepositoryRoot, ".editorconfig"), cancellationToken) + )[0]; + + await Assert.That(firstLine.StartsWith(marker, StringComparison.Ordinal)).IsTrue(); + } + + /// + /// Drives the real pack-time stamping target and asserts it writes the version it was packed with. + /// + /// + /// + /// This is the mechanism that keeps a consumer's bootstrapped .editorconfig honest, so it is + /// worth testing against the target itself rather than against its effect on one particular pack. The + /// version asked for is deliberately not the repository's: a stamp that happens to agree with the + /// tracked file proves nothing. + /// + /// + /// The tracked .editorconfig must come back unchanged. A build that rewrites a tracked source + /// file leaves a dirty working tree on CI and races when projects build in parallel. + /// + /// + [Test] + public async Task StampTarget_WritesThePackedVersionAndLeavesTheTrackedFileAlone( + CancellationToken cancellationToken + ) + { + const string stampedVersion = "4.5.6-test"; + + var projectPath = Path.Combine(RepositoryRoot, "src", "src", "BuildSdk", "BuildSdk.csproj"); + var trackedPath = Path.Combine(RepositoryRoot, ".editorconfig"); + var trackedBefore = await File.ReadAllTextAsync(trackedPath, cancellationToken); + + var scratchDirectory = Path.Combine(Path.GetTempPath(), "purview-stamp-" + Guid.NewGuid().ToString("N")); + var destination = Path.Combine(scratchDirectory, ".editorconfig"); + + try + { + var (exitCode, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( + $"msbuild \"{projectPath}\" -t:StampPurviewEditorConfigVersionTarget -p:PackageVersion={stampedVersion} \"-p:_PurviewStampedEditorConfig={destination}\" -v:m", + RepositoryRoot, + cancellationToken + ); + var output = stdOut + stdErr; + + await Assert.That(exitCode).IsEqualTo(0).Because(output); + await Assert.That(File.Exists(destination)).IsTrue().Because(output); + + var stamped = await File.ReadAllLinesAsync(destination, cancellationToken); + await Assert.That(stamped[0]).IsEqualTo($"# Purview.BuildSdk {stampedVersion} - managed file, do not edit"); + + // Only the marker is rewritten; the rules themselves are packed verbatim. + var trackedLines = await File.ReadAllLinesAsync(trackedPath, cancellationToken); + await Assert.That(stamped.Length).IsEqualTo(trackedLines.Length); + await Assert.That(stamped.Skip(1)).IsEquivalentTo(trackedLines.Skip(1)); + } + finally + { + if (Directory.Exists(scratchDirectory)) + Directory.Delete(scratchDirectory, recursive: true); + } + + await Assert.That(await File.ReadAllTextAsync(trackedPath, cancellationToken)).IsEqualTo(trackedBefore); + } + + /// + /// The quick-start snippets and the starter template are what somebody adopting the SDK copies, so a + /// stale pin there is adopted verbatim. + /// + [Test] + [Arguments("README.md")] + [Arguments("docs/wiki/Getting-Started.md")] + [Arguments("templates/global.json")] + public async Task AdoptionPin_MatchesPackageJson(string relativePath, CancellationToken cancellationToken) + { + var path = Path.Combine(RepositoryRoot, relativePath.Replace('/', Path.DirectorySeparatorChar)); + var content = await File.ReadAllTextAsync(path, cancellationToken); + + var pins = BuildSdkPin() + .Matches(content) + .Select(match => match.Groups["version"].Value) + // The property reference in the auto-detection documentation is not a pin. + .Where(version => !version.StartsWith('$')) + .ToArray(); + + await Assert.That(pins).IsNotEmpty(); + + foreach (var pin in pins) + await Assert.That(pin).IsEqualTo(PackageVersion()); + } +} diff --git a/src/tests/BuildSdk.IntegrationTests/ProjectIdentityRulesTests.cs b/src/tests/BuildSdk.IntegrationTests/ProjectIdentityRulesTests.cs index a2216ea..fc02659 100644 --- a/src/tests/BuildSdk.IntegrationTests/ProjectIdentityRulesTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/ProjectIdentityRulesTests.cs @@ -1,4 +1,3 @@ -using System.Diagnostics; using System.Text.Json; using Purview.BuildSdk.Harness; using Purview.BuildSdk.Infra; @@ -518,11 +517,10 @@ await File.WriteAllTextAsync( cancellationToken ); - var (exitCode, stdOut, stdErr) = await RunProcessAsync( - "dotnet", + var (exitCode, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( $"msbuild \"{projectFilePath}\" -nologo -t:ValidateProjectFileNamingConventionTarget", projectDirectory, - cancellationToken + cancellationToken: cancellationToken ); return (exitCode, stdOut + stdErr); @@ -565,33 +563,4 @@ static List ExtractItemMetadataValues(string msbuildOutput, string itemT return []; } - - static async Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( - string fileName, - string arguments, - string workingDirectory, - CancellationToken cancellationToken - ) - { - using var process = new Process - { - StartInfo = new ProcessStartInfo - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - process.Start(); - var stdOutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stdErrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdOutTask, await stdErrTask); - } } diff --git a/src/tests/BuildSdk.IntegrationTests/PurviewSkillInstallCommandTests.cs b/src/tests/BuildSdk.IntegrationTests/PurviewSkillInstallCommandTests.cs index b45d277..ed58eb6 100644 --- a/src/tests/BuildSdk.IntegrationTests/PurviewSkillInstallCommandTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PurviewSkillInstallCommandTests.cs @@ -360,7 +360,7 @@ public async Task RepositoryRoot_LetsTheDefinitiveMarkerBeatANearerOne(Cancellat Directory.CreateDirectory(Path.Combine(harness.SolutionDirectory, ".git")); await File.WriteAllTextAsync( Path.Combine(harness.ProjectDirectory, "package.json"), - "{ \"name\": \"nested\" }\n", + /*lang=json,strict*/"{ \"name\": \"nested\" }\n", cancellationToken ); @@ -383,7 +383,7 @@ CancellationToken cancellationToken Directory.CreateDirectory(Path.Combine(harness.SolutionDirectory, ".github")); await File.WriteAllTextAsync( Path.Combine(harness.ProjectDirectory, "package.json"), - "{ \"name\": \"nested\" }\n", + /*lang=json,strict*/"{ \"name\": \"nested\" }\n", cancellationToken ); @@ -404,7 +404,7 @@ public async Task RepositoryRoot_BreaksTiesWithinADirectoryByListOrder(Cancellat await File.WriteAllTextAsync(Path.Combine(harness.SolutionDirectory, "LICENSE.md"), "MIT\n", cancellationToken); await File.WriteAllTextAsync( Path.Combine(harness.SolutionDirectory, "package.json"), - "{ \"name\": \"root\" }\n", + /*lang=json,strict*/"{ \"name\": \"root\" }\n", cancellationToken ); diff --git a/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs b/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs index bef3472..a64d628 100644 --- a/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PurviewSkillRecommendationTests.cs @@ -309,6 +309,8 @@ public async Task TheBuildHook_ReportsEachDistinctStateOnce(CancellationToken ca ); 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"); diff --git a/src/tests/BuildSdk.IntegrationTests/PurviewSkillResolutionTests.cs b/src/tests/BuildSdk.IntegrationTests/PurviewSkillResolutionTests.cs index 63fb4e2..323a60b 100644 --- a/src/tests/BuildSdk.IntegrationTests/PurviewSkillResolutionTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PurviewSkillResolutionTests.cs @@ -60,6 +60,7 @@ CancellationToken cancellationToken return []; } + // The MSBuild task writes a JSON array of objects, each of which is a dictionary of string properties. return [ .. entries diff --git a/src/tests/BuildSdk.IntegrationTests/PurviewSkillValidationTests.cs b/src/tests/BuildSdk.IntegrationTests/PurviewSkillValidationTests.cs index 187c460..a808709 100644 --- a/src/tests/BuildSdk.IntegrationTests/PurviewSkillValidationTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/PurviewSkillValidationTests.cs @@ -179,7 +179,47 @@ public async Task ValidatePurviewSkills_ChecksEveryAuthoredSkill(CancellationTok [Test] public async Task Pack_FailsWhenAPackedSkillIsMalformed(CancellationToken cancellationToken) { - using var harness = await ProjectHarness.CreateAsync( + using var harness = await CreatePackHarnessAsync(cancellationToken); + await WriteSkillAsync(harness, "purview-broken", "# no frontmatter\n", cancellationToken); + + var (exitCode, stdOut, stdErr) = await PackAsync(harness, cancellationToken); + + // The repository that owns the skill is the one that sees the failure, rather than shipping + // a skill the CLI will silently skip. + await Assert.That(exitCode).IsNotEqualTo(0).Because(TestHelpers.GenerateError(stdOut, stdErr)); + await Assert.That(stdOut + stdErr).Contains("PRSKL0005"); + } + + /// + /// A CI runner exports GITHUB_WORKSPACE, and the SDK's version detection reads + /// RepoRoot from it. The throwaway project is not inside that workspace, so inheriting the + /// variable must not make pack validate the *checkout's* skills while the project's own malformed + /// one goes unnoticed. This is how the test above first failed on the Linux runner. + /// + [Test] + public async Task Pack_ValidatesTheProjectsOwnSkills_EvenWhenAGitHubWorkspaceIsInherited( + CancellationToken cancellationToken + ) + { + using var harness = await CreatePackHarnessAsync( + cancellationToken, + new Dictionary(StringComparer.OrdinalIgnoreCase) { ["GITHUB_WORKSPACE"] = RepositoryRoot } + ); + await WriteSkillAsync(harness, "purview-broken", "# no frontmatter\n", cancellationToken); + + var (exitCode, stdOut, stdErr) = await PackAsync(harness, cancellationToken); + + await Assert.That(exitCode).IsNotEqualTo(0).Because(TestHelpers.GenerateError(stdOut, stdErr)); + await Assert.That(stdOut + stdErr).Contains("PRSKL0005"); + await Assert.That(stdOut + stdErr).Contains("purview-broken"); + } + + static async Task CreatePackHarnessAsync( + CancellationToken cancellationToken, + IDictionary? extraEnv = null + ) + { + var harness = await ProjectHarness.CreateAsync( "SkillValidationPack", extraProps: """ true @@ -187,6 +227,7 @@ public async Task Pack_FailsWhenAPackedSkillIsMalformed(CancellationToken cancel true true """, + extraEnv: extraEnv, cancellationToken: cancellationToken ); await File.WriteAllTextAsync( @@ -194,48 +235,49 @@ await File.WriteAllTextAsync( "# Agents", cancellationToken ); - await WriteSkillAsync(harness, "purview-broken", "# no frontmatter\n", cancellationToken); + return harness; + } + /// + /// Packs through the harness so the runner's environment cannot leak in: a spawned process that + /// inherits GITHUB_WORKSPACE resolves the wrong repository root and validates the wrong + /// skills. + /// + static Task<(int Code, string StdOut, string StdErr)> PackAsync( + ProjectHarness harness, + CancellationToken cancellationToken + ) + { var feedDirectory = Path.Combine(harness.SolutionDirectory, "feed"); - var (exitCode, stdOut, stdErr) = await RunProcessAsync( + + return harness.RunProcessAsync( "dotnet", $"pack \"{harness.ProjectFilePath}\" -c Release -o \"{feedDirectory}\" -p:PackageVersion=0.0.0-test -p:Version=0.0.0-test", harness.SolutionDirectory, cancellationToken ); - - // The repository that owns the skill is the one that sees the failure, rather than shipping - // a skill the CLI will silently skip. - await Assert.That(exitCode).IsNotEqualTo(0); - await Assert.That(stdOut + stdErr).Contains("PRSKL0005"); } - static async Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( - string fileName, - string arguments, - string workingDirectory, - CancellationToken cancellationToken - ) + /// + /// This repository's root, found by the same AGENTS.md marker the SDK uses, so a test can + /// point GITHUB_WORKSPACE at a real checkout. + /// + static string RepositoryRoot { - using var process = new System.Diagnostics.Process + get { - StartInfo = new System.Diagnostics.ProcessStartInfo + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null) { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - process.Start(); - var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdoutTask, await stderrTask); + if (File.Exists(Path.Combine(directory.FullName, "AGENTS.md"))) + return directory.FullName; + + directory = directory.Parent; + } + + throw new InvalidOperationException( + $"No AGENTS.md was found above '{AppContext.BaseDirectory}', so the repository root could not be resolved." + ); + } } } diff --git a/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs b/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs index 8ee06be..d7c269e 100644 --- a/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/RepositoryFileSyncTests.cs @@ -59,6 +59,139 @@ public async Task EditorConfigBootstrap_ExistingFile_IsNeverOverwritten(Cancella await Assert.That(await File.ReadAllTextAsync(editorConfigPath, cancellationToken)).IsEqualTo("# user owned\n"); } + /// + /// 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. + /// + [Test] + public async Task EditorConfigBootstrap_StaleVersionMarker_IsUpdated(CancellationToken cancellationToken) + { + using var h = await CreateHarnessAsync(extraProps: null, cancellationToken: cancellationToken); + + var editorConfigPath = Path.Combine(h.SolutionDirectory, ".editorconfig"); + await File.WriteAllTextAsync( + editorConfigPath, + "# Purview.BuildSdk 0.0.1 - managed file, do not edit\nroot = true\n", + cancellationToken + ); + + var (success, output, errors) = await h.BuildAsync(restore: true, verbose: true, cancellationToken); + + await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); + + var updated = await File.ReadAllTextAsync(editorConfigPath, cancellationToken); + await Assert.That(updated).DoesNotContain("Purview.BuildSdk 0.0.1"); + await Assert.That(updated).Contains("Purview.BuildSdk "); + } + + /// + /// The marker is the whole of the ownership test. Without it the file is the developer's, whatever it + /// contains, and must survive untouched — which is what makes deleting the marker line a supported way + /// to take ownership. + /// + [Test] + public async Task EditorConfigBootstrap_NoVersionMarker_IsLeftAlone(CancellationToken cancellationToken) + { + using var h = await CreateHarnessAsync(extraProps: null, cancellationToken: cancellationToken); + + var editorConfigPath = Path.Combine(h.SolutionDirectory, ".editorconfig"); + const string owned = "root = true\n\n[*.cs]\nindent_style = space\n"; + + await File.WriteAllTextAsync(editorConfigPath, owned, cancellationToken); + + var (success, output, errors) = await h.BuildAsync(restore: true, verbose: true, cancellationToken); + + await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); + await Assert.That(await File.ReadAllTextAsync(editorConfigPath, cancellationToken)).IsEqualTo(owned); + } + + /// + /// An unmarked file is left alone, but silence is how a repository bootstrapped before the marker + /// existed stays pinned to a release-old rule set forever. It is reported once per SDK version, so + /// the second build of the same state has to say nothing. + /// + [Test] + public async Task EditorConfigBootstrap_NoVersionMarker_IsReportedOnce(CancellationToken cancellationToken) + { + using var h = await CreateHarnessAsync( + // The suite itself runs in CI, where the advice is deliberately silent. + extraProps: "true", + cancellationToken: cancellationToken + ); + + var editorConfigPath = Path.Combine(h.SolutionDirectory, ".editorconfig"); + const string owned = "root = true\n\n[*.cs]\nindent_style = space\n"; + + await File.WriteAllTextAsync(editorConfigPath, owned, cancellationToken); + + var (success, output, errors) = await h.BuildAsync(restore: true, verbose: true, cancellationToken); + + await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); + await Assert.That(output + errors).Contains("carries no Purview.BuildSdk version marker"); + await Assert.That(output + errors).Contains("PurviewRepoBootstrapMode=Always"); + await Assert.That(await File.ReadAllTextAsync(editorConfigPath, cancellationToken)).IsEqualTo(owned); + + var (secondSuccess, secondOutput, secondErrors) = await h.BuildAsync( + restore: false, + verbose: true, + cancellationToken + ); + + await Assert.That(secondSuccess).IsTrue().Because(TestHelpers.GenerateError(secondOutput, secondErrors)); + await Assert.That(secondOutput + secondErrors).DoesNotContain("carries no Purview.BuildSdk version marker"); + } + + /// + /// Taking ownership of the file is the supported outcome, so the property that says so has to + /// silence the advice as well as the refresh. + /// + [Test] + public async Task EditorConfigBootstrap_AutoUpdateDisabled_SuppressesTheAdoptionWarning( + CancellationToken cancellationToken + ) + { + using var h = await CreateHarnessAsync( + extraProps: """ + true + false + """, + cancellationToken: cancellationToken + ); + + await File.WriteAllTextAsync( + Path.Combine(h.SolutionDirectory, ".editorconfig"), + "root = true\n", + cancellationToken + ); + + var (success, output, errors) = await h.BuildAsync(restore: true, verbose: true, cancellationToken); + + await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); + await Assert.That(output + errors).DoesNotContain("carries no Purview.BuildSdk version marker"); + } + + /// + /// The documented opt-out has to actually pin the file, marker and all. + /// + [Test] + public async Task EditorConfigBootstrap_AutoUpdateDisabled_KeepsTheStaleFile(CancellationToken cancellationToken) + { + using var h = await CreateHarnessAsync( + extraProps: "false", + cancellationToken: cancellationToken + ); + + var editorConfigPath = Path.Combine(h.SolutionDirectory, ".editorconfig"); + const string stale = "# Purview.BuildSdk 0.0.1 - managed file, do not edit\nroot = true\n"; + + await File.WriteAllTextAsync(editorConfigPath, stale, cancellationToken); + + var (success, output, errors) = await h.BuildAsync(restore: true, verbose: true, cancellationToken); + + await Assert.That(success).IsTrue().Because(TestHelpers.GenerateError(output, errors)); + await Assert.That(await File.ReadAllTextAsync(editorConfigPath, cancellationToken)).IsEqualTo(stale); + } + [Test] public async Task EditorConfigBootstrap_WarnOnDrift_ReportsWarningAndKeepsUserFile( CancellationToken cancellationToken @@ -263,6 +396,10 @@ static async Task CreateHarnessAsync(string? extraProps, Cancell """, + // These are the tests that cover the bootstrap, so the solution has to start without the files + // it creates. Every other harness test seeds them, to keep the bootstrap's inline task out of + // the build. + seedRepositoryBootstrapFiles: false, cancellationToken: cancellationToken ); diff --git a/src/tests/BuildSdk.IntegrationTests/RepositorySkillsSelfTests.cs b/src/tests/BuildSdk.IntegrationTests/RepositorySkillsSelfTests.cs index b17e04d..d1412f1 100644 --- a/src/tests/BuildSdk.IntegrationTests/RepositorySkillsSelfTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/RepositorySkillsSelfTests.cs @@ -1,5 +1,6 @@ using System.Text.Json; using System.Text.RegularExpressions; +using Purview.BuildSdk.Infra; namespace Purview.BuildSdk; @@ -87,6 +88,7 @@ static LockEntry[] ReadLockFile() if (!document.RootElement.TryGetProperty("skills", out var skills)) return []; + // The lock file is a dictionary of skill names to objects with "source" and "sourceType" properties. return [ .. skills @@ -195,25 +197,18 @@ await Assert static async Task TrackedFilesUnderAsync(string repositoryRelativePath) { - using var process = System.Diagnostics.Process.Start( - new System.Diagnostics.ProcessStartInfo("git", $"ls-files -- \"{repositoryRelativePath}\"") - { - WorkingDirectory = RepositoryRoot, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - } + var (exitCode, stdOut, _) = await ProcessRunner.RunAsync( + "git", + $"ls-files -- \"{repositoryRelativePath}\"", + RepositoryRoot ); - if (process is null) + if (exitCode != 0) return []; - var output = await process.StandardOutput.ReadToEndAsync(); - await process.WaitForExitAsync(); - return [ - .. output + .. stdOut .Split('\n', StringSplitOptions.RemoveEmptyEntries) .Select(line => line.Trim()) .Where(line => line.Length > 0), diff --git a/src/tests/BuildSdk.IntegrationTests/SdkPackageConsumptionTests.cs b/src/tests/BuildSdk.IntegrationTests/SdkPackageConsumptionTests.cs index 1fd47de..9b84ae7 100644 --- a/src/tests/BuildSdk.IntegrationTests/SdkPackageConsumptionTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/SdkPackageConsumptionTests.cs @@ -1,7 +1,5 @@ -using System.Diagnostics; using System.IO.Compression; using System.Text.Json; -using Purview.BuildSdk.Harness; using Purview.BuildSdk.Infra; namespace Purview.BuildSdk; @@ -221,7 +219,7 @@ CancellationToken cancellationToken { var nugetConfigPath = Path.Combine(consumerDirectory, "NuGet.Config"); - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"msbuild \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -noconlog -p:RestoreConfigFile=\"{nugetConfigPath}\" -getItem:Analyzer -p:CentralPackageFloatingVersionsEnabled=true {extraMsbuildArgs}", consumerDirectory, @@ -250,7 +248,7 @@ static async Task VerifyPds0003WarningOnBuildAsync(string consumerDirectory, Can { var nugetConfigPath = Path.Combine(consumerDirectory, "NuGet.Config"); - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"build \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -p:RestoreConfigFile=\"{nugetConfigPath}\" -p:CentralPackageFloatingVersionsEnabled=true -p:NoWarn=NU1010", consumerDirectory, @@ -271,7 +269,7 @@ static async Task PackSdkAsync(string feedDirectory, CancellationToken c ?? throw new InvalidOperationException("Unable to determine SDK project directory."); var packageVersion = $"0.0.0-integration-test-{Guid.NewGuid():N}"; - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"pack \"{sdkProjectPath}\" -c Release -o \"{feedDirectory}\" -p:PackageVersion={packageVersion} -p:Version={packageVersion}", sdkProjectDirectory, @@ -390,7 +388,7 @@ static async Task SetupConsumerProjectAsync( CancellationToken cancellationToken ) { - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", "new sln -n Proof", consumerDirectory, @@ -398,10 +396,10 @@ CancellationToken cancellationToken ); await Assert.That(code).IsEqualTo(0).Because(TestHelpers.GenerateError(stdOut, stdErr)); - (code, stdOut, stdErr) = await RunProcessAsync("git", "init", consumerDirectory, cancellationToken); + (code, stdOut, stdErr) = await ProcessRunner.RunAsync("git", "init", consumerDirectory, cancellationToken); await Assert.That(code).IsEqualTo(0).Because(TestHelpers.GenerateError(stdOut, stdErr)); - (code, stdOut, stdErr) = await RunProcessAsync( + (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"new classlib -n Proof.LibTest -o \"{Path.Combine(consumerSrcDirectory, "Proof.LibTest")}\" -f net10.0", consumerDirectory, @@ -418,7 +416,7 @@ await Assert .IsNotNull() .Because("The generated solution file was not found in the consumer directory."); - (code, stdOut, stdErr) = await RunProcessAsync( + (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"sln \"{solutionPath}\" add \"{Path.Combine(consumerSrcDirectory, "Proof.LibTest", "Proof.LibTest.csproj")}\"", consumerDirectory, @@ -514,7 +512,7 @@ static async Task VerifyEditorConfigIntegrationAsync(string consumerDirectory, C { var nugetConfigPath = Path.Combine(consumerDirectory, "NuGet.Config"); - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"msbuild \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -noconlog -p:RestoreConfigFile=\"{nugetConfigPath}\" -getProperty:EditorConfigFilePath -getItem:EditorConfigFiles -p:CentralPackageFloatingVersionsEnabled=true", consumerDirectory, @@ -560,7 +558,7 @@ await Assert .IsTrue(); var editorConfigContent = await File.ReadAllTextAsync(editorConfigPath!, cancellationToken); - (code, stdOut, stdErr) = await RunProcessAsync( + (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"build \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -p:RestoreConfigFile=\"{nugetConfigPath}\" -p:CentralPackageFloatingVersionsEnabled=true -p:NoWarn=NU1010", consumerDirectory, @@ -584,7 +582,7 @@ await Assert .IsFalse() .Because($"A build created '{consumerAgentsFolder}'; only the skills CLI may write there."); - var (code, stdOut, stdErr) = await RunProcessAsync( + var (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"msbuild \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -p:RestoreConfigFile=\"{nugetConfigPath}\" -t:EnsureRepositoryEditorConfigTarget -getProperty:RepositoryEditorConfigFilePath", consumerDirectory, @@ -609,7 +607,7 @@ await Assert .Because($"Repository EditorConfig file not found at path: {repositoryEditorConfigPath}"); File.Delete(repositoryEditorConfigPath); - (code, stdOut, stdErr) = await RunProcessAsync( + (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"msbuild \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -p:RestoreConfigFile=\"{nugetConfigPath}\" -p:BootstrapEditorConfigToRepoRoot=false -t:EnsureRepositoryEditorConfigTarget", consumerDirectory, @@ -621,7 +619,7 @@ await Assert .IsFalse() .Because("BootstrapEditorConfigToRepoRoot=false should disable the physical repo-level copy."); - (code, stdOut, stdErr) = await RunProcessAsync( + (code, stdOut, stdErr) = await ProcessRunner.RunAsync( "dotnet", $"msbuild \"{Path.Combine("src", "Proof.LibTest", "Proof.LibTest.csproj")}\" -nologo -p:RestoreConfigFile=\"{nugetConfigPath}\" -t:EnsureRepositoryGlobalJsonTarget -getProperty:RepositoryGlobalJsonFilePath", consumerDirectory, @@ -658,41 +656,4 @@ await Assert $"Repository GlobalJson file at path {repositoryGlobalJsonPath} does not contain the expected content." ); } - - static async Task<(int Code, string StdOut, string StdErr)> RunProcessAsync( - string fileName, - string arguments, - string workingDirectory, - CancellationToken cancellationToken - ) - { - using var process = new Process - { - StartInfo = new ProcessStartInfo - { - FileName = fileName, - Arguments = arguments, - WorkingDirectory = workingDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - // Isolate the throwaway consumer from the host CI environment so repo-root discovery - // (and therefore the .agents copy destination) resolves inside the consumer directory - // rather than GITHUB_WORKSPACE on CI runners. - ProjectHarness.IsolateFromHostEnvironment( - process.StartInfo.Environment, - new Dictionary(StringComparer.OrdinalIgnoreCase) - ); - - process.Start(); - var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, await stdoutTask, await stderrTask); - } } diff --git a/src/tests/BuildSdk.IntegrationTests/VersionDetectionTests.cs b/src/tests/BuildSdk.IntegrationTests/VersionDetectionTests.cs index eefa1b8..ed4ef52 100644 --- a/src/tests/BuildSdk.IntegrationTests/VersionDetectionTests.cs +++ b/src/tests/BuildSdk.IntegrationTests/VersionDetectionTests.cs @@ -1,4 +1,5 @@ using Purview.BuildSdk.Harness; +using Purview.BuildSdk.Infra; namespace Purview.BuildSdk; @@ -431,8 +432,8 @@ await File.WriteAllTextAsync( } /// - /// Runs dotnet msbuild against the throwaway project with the host CI environment - /// stripped out (GITHUB_WORKSPACE, GITHUB_ACTIONS, etc.), so version detection and CI + /// Runs dotnet msbuild against the throwaway project through the shared runner, which strips + /// the host CI environment (GITHUB_WORKSPACE, GITHUB_ACTIONS, etc.), so version detection and CI /// detection behave deterministically on every machine and CI runner. /// static async Task<(int ExitCode, string Output)> RunMSBuildAsync( @@ -441,30 +442,12 @@ await File.WriteAllTextAsync( CancellationToken cancellationToken ) { - using var process = new System.Diagnostics.Process - { - StartInfo = new System.Diagnostics.ProcessStartInfo - { - FileName = "dotnet", - Arguments = $"msbuild \"{harness.ProjectFilePath}\" -nologo {msbuildArguments}", - WorkingDirectory = harness.ProjectDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - CreateNoWindow = true, - }, - }; - - ProjectHarness.IsolateFromHostEnvironment( - process.StartInfo.Environment, - new Dictionary(StringComparer.OrdinalIgnoreCase) - ); - - process.Start(); - var stdoutTask = process.StandardOutput.ReadToEndAsync(cancellationToken); - var stderrTask = process.StandardError.ReadToEndAsync(cancellationToken); - await process.WaitForExitAsync(cancellationToken); - - return (process.ExitCode, (await stdoutTask) + (await stderrTask)); + var (code, stdOut, stdErr) = await ProcessRunner.RunDotNetAsync( + $"msbuild \"{harness.ProjectFilePath}\" -nologo {msbuildArguments}", + harness.ProjectDirectory, + cancellationToken + ); + + return (code, stdOut + stdErr); } } diff --git a/templates/Directory.Packages.props b/templates/Directory.Packages.props index 3797168..c7d3da4 100644 --- a/templates/Directory.Packages.props +++ b/templates/Directory.Packages.props @@ -2,53 +2,53 @@ - - true + 13.6.2 + 10.0.402 + 1.76.0 - - - - - + + + + + - - - + + + - - + + - + - - - - + + + - - + + - - - + + + diff --git a/templates/global.json b/templates/global.json index dfd4bfe..838474b 100644 --- a/templates/global.json +++ b/templates/global.json @@ -3,6 +3,6 @@ "runner": "Microsoft.Testing.Platform" }, "msbuild-sdks": { - "Purview.BuildSdk": "1.0.0" + "Purview.BuildSdk": "1.1.0" } }