fix(scribe): ci broke#13
Conversation
There was a problem hiding this comment.
Pull request overview
Updates LocalDev and Scribe.Sdk build infrastructure to address restore/CI fragility and simplify phase gating, while standardizing the LocalDev sentinel naming convention.
Changes:
- Standardize LocalDev activation sentinels to a single
.<producer>.scribefile and update.gitignore/docs accordingly. - Prevent restore failures by only adding the local packages folder as a NuGet source when it exists.
- Gate Scribe.Sdk per-type MSBuild phase imports at the
<Import>level to reduce per-node conditional drift.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| version.json | Bumps NBGV version from 0.4 to 0.5. |
| Scribe/build/Scribe.LocalDev.targets | Updates sentinel wording; retains LocalDev override/cleanup behavior. |
| Scribe/build/Scribe.LocalDev.props | Switches sentinel scanning to *.scribe and guards restore source registration with Exists(...). |
| Scribe.slnx | Adds version.json under Solution Items. |
| Scribe.Sdk/Sdk/Sdk.targets | Conditions per-type phase imports to only load relevant phase files. |
| Scribe.Sdk/Sdk/Phases/Stubs.targets | Removes redundant per-node condition now that import is gated. |
| Scribe.Sdk/Sdk/Phases/Meta.targets | Removes redundant per-node conditions now that import is gated. |
| Scribe.Sdk/Sdk/Phases/Library.targets | Removes redundant per-node conditions now that import is gated. |
| Scribe.Sdk/Sdk/Phases/Companion.targets | Removes redundant per-node conditions now that import is gated. |
| Scribe.Sdk/Sdk/Phases/Analyzer.targets | Removes redundant per-node conditions now that import is gated. |
| docs/project-setup.md | Updates LocalDev sentinel documentation to .scribe. |
| docs/architecture-infrastructure.md | Updates LocalDev sentinel documentation to .scribe. |
| Directory.Build.props | Updates LocalDev sentinel comment to .scribe. |
| .gitignore | Replaces broad *.user/*.local ignores with a single *.scribe rule. |
| $(ScribesName) Producer project name. Used for: | ||
| • sentinel detection: '.$(ScribesName).user' or '.$(ScribesName).local' | ||
| • sentinel detection: '.$(ScribesName).scribe' | ||
| • auto-pack trigger: $(MSBuildProjectName) == $(ScribesName) | ||
| • override file name: $(ScribesName).Directory.Packages.targets |
There was a problem hiding this comment.
The comments here say the override file name is based on
| - **Sentinel detection:** matches `.$(ScribesName).user` OR `.$(ScribesName).local` (lowercased, e.g. `.myframework.user` or `.myframework.local`) at `$(ScribeRoot)`. Either form activates — pick whichever you prefer. | ||
| - **Sentinel detection:** matches `.$(ScribesName).scribe` (lowercased, e.g. `.myframework.scribe`) at `$(ScribeRoot)`. | ||
| - **Auto-pack trigger:** the MSBuild project whose name equals `$(ScribesName)` is the one that packs. | ||
| - **Override file name:** generated as `$(ScribesName).Directory.Packages.targets`. |
There was a problem hiding this comment.
This section documents the override file as '$(ScribesName).Directory.Packages.targets', but LocalDev actually emits per-package override files named '$(PackageId).Directory.Packages.targets' (see Scribe/build/Scribe.LocalDev.targets). Please update the docs to match the real file naming behavior, especially for multi-package producers.
| - **Override file name:** generated as `$(ScribesName).Directory.Packages.targets`. | |
| - **Override file names:** generated per package as `$(PackageId).Directory.Packages.targets` for each package ID in `$(ScribeLocalDevPackageNames)`. |
| <ItemGroup> | ||
| <_ScribeLocalSentinel Include="$(ScribeRoot)\.*.user" /> | ||
| <_ScribeLocalSentinel Include="$(ScribeRoot)\.*.local" /> | ||
| <_ScribeLocalSentinel Include="$(ScribeRoot)\.*.scribe" /> |
There was a problem hiding this comment.
The umbrella sentinel scan example uses backslashes in the glob ('$(ScribeRoot)..scribe'), but Scribe.LocalDev.props uses forward slashes for globs ('$(ScribeRoot)/..scribe') to behave consistently on Linux/Windows. Update the snippet (and the nearby Path Setup restore-source example) to reflect the actual implementation, including the new 'only add RestoreAdditionalProjectSources when the packages directory exists' behavior.
| <_ScribeLocalSentinel Include="$(ScribeRoot)\.*.scribe" /> | |
| <_ScribeLocalSentinel Include="$(ScribeRoot)/.*.scribe" /> |
| Three activation methods (umbrella): | ||
|
|
||
| 1. **Sentinel file (recommended):** Create `.<producer>.user` OR `.<producer>.local` in `$(ScribeRoot)` — e.g. `.scribe.user` to build Scribe locally, `.hermetic.local` to build Hermetic locally. Either form works. Delete to deactivate. Add to `.gitignore`. | ||
| 1. **Sentinel file (recommended):** Create `.<producer>.scribe` in `$(ScribeRoot)` — e.g. `.scribe.scribe` to build Scribe locally, `.hermetic.scribe` to build Hermetic locally. Delete to deactivate. Add to `.gitignore` via `*.scribe`. | ||
|
|
||
| **Why two forms?** `.user` is the .NET convention — `*.user` is in the standard [VisualStudio.gitignore](https://github.com/github/gitignore/blob/main/VisualStudio.gitignore), so it won't be accidentally committed. `.local` reads more naturally and matches the JS ecosystem (Vite, Next.js). Both are accepted rather than forcing a choice between idioms. | ||
| **Why `.scribe`?** The suffix is brand-unique — zero risk of collision with `.user` (VS project user settings) or `.local` (Vite/Next.js/prettier env files) that a future tool might also scan. One `.gitignore` line covers every producer. | ||
|
|
||
| 2. **Explicit property:** `-p:IsLocalScribe=true` on the command line — consumer infra only. Does not activate any specific producer; pair with a `-p:IsLocalProducer=true` or a sentinel. | ||
| 3. **Props file:** `<IsLocalScribe>true</IsLocalScribe>` in Directory.Build.props or Directory.Solution.props. |
There was a problem hiding this comment.
There’s still a reference later in this doc to removing a producer’s '..user' / '..local' sentinel to trigger cleanup (see 'Cleanup on Sentinel Removal' section). Since LocalDev now standardizes on '..scribe', that section should be updated to avoid conflicting instructions.
| #### 4. Activate | ||
|
|
||
| Activation is **per-producer**: | ||
|
|
||
| 1. **Sentinel file (recommended):** Create EITHER `.$(ScribesName).user` OR `.$(ScribesName).local` at `$(ScribeRoot)` — for example `.myframework.user` or `.myframework.local`. Either form works; pick whichever you prefer. Delete to deactivate. Multiple producers can be activated independently (e.g. `.scribe.user` + `.hermetic.local`). Already `.gitignore`d via the `*.user` and `*.local` patterns. | ||
| 1. **Sentinel file (recommended):** Create `.$(ScribesName).scribe` at `$(ScribeRoot)` — for example `.myframework.scribe`. Delete to deactivate. Multiple producers can be activated independently (e.g. `.scribe.scribe` + `.hermetic.scribe`). Already `.gitignore`d via a single `*.scribe` pattern. | ||
|
|
||
| **Why two forms?** `.user` is the long-standing .NET convention — `*.user` is already in the standard [VisualStudio.gitignore](https://github.com/github/gitignore/blob/main/VisualStudio.gitignore) so it won't be accidentally committed in any .NET repo. `.local` reads more naturally ("myframework, local variant") and matches the convention used by the JS ecosystem (Vite, Next.js, etc.). Rather than force a choice between "idiomatic .NET" and "idiomatic elsewhere," both are accepted. | ||
| **Why `.scribe`?** The suffix is brand-unique — zero risk of collision with `.user` (VS project user settings) or `.local` (Vite/Next.js/prettier env files) that a future tool might also scan. One `.gitignore` line (`*.scribe`) covers every producer regardless of name. |
There was a problem hiding this comment.
The overview of how the consumer resolves local packages implies the shared packages directory is always registered as a NuGet source, but LocalDev.props now only appends it when the directory exists (to avoid NU1301 on fresh clones/CI). Consider adding a note here that the producer must pack once (creating '.artifacts/packages') before restores will pick up the local source.
No description provided.