feat(scribe): design for BulletsForHumanity.Scribe.Sdk MSBuild SDK package#6
Conversation
Agent-Logs-Url: https://github.com/BulletsForHumanity/Scribe/sessions/be6b1a11-f5a6-49dd-a843-ea8134bb35d1 Co-authored-by: crazycrank <11194014+crazycrank@users.noreply.github.com>
…or Scribe SDK Agent-Logs-Url: https://github.com/BulletsForHumanity/Scribe/sessions/be6b1a11-f5a6-49dd-a843-ea8134bb35d1 Co-authored-by: crazycrank <11194014+crazycrank@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Introduces a new NuGet-delivered MSBuild SDK (BulletsForHumanity.Scribe.Sdk) to standardize and simplify setup/packaging for Scribe-based Roslyn analyzers and source generators, and updates repository/docs to reflect the new recommended workflow.
Changes:
- Adds
Scribe.Sdk/pack-only project that shipsSdk.props/Sdk.targets, LocalDev infrastructure, andStubs.cspolyfills. - Implements analyzer packaging + dependency bundling behavior via SDK MSBuild targets.
- Updates solution + documentation (README, project setup, infrastructure architecture) to recommend SDK-based setup.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Scribe.slnx | Adds the new Scribe.Sdk project to the solution. |
| Scribe.Sdk/Sdk/Sdk.props | Sets early-phase defaults for analyzer/generator projects and imports LocalDev props. |
| Scribe.Sdk/Sdk/Sdk.targets | Adds late-phase packaging targets, dependency bundling, and imports LocalDev targets. |
| Scribe.Sdk/Scribe.Sdk.csproj | Pack-only project that packages SDK assets, stubs, LocalDev files, and metadata. |
| Scribe.Sdk/README.md | Adds SDK-specific usage and behavior documentation. |
| Scribe.Sdk/content/Stubs.cs | Provides netstandard2.0 polyfill stubs injected by the SDK. |
| README.md | Updates “Getting Started” to recommend SDK usage. |
| docs/project-setup.md | Adds an SDK-based setup path and preserves manual setup guidance. |
| docs/architecture-infrastructure.md | Documents SDK package layout, resolution, and phase behavior alongside LocalDev. |
| and '%(ReferenceCopyLocalPaths.NuGetPackageId)' != 'Microsoft.CodeAnalysis.CSharp' | ||
| and '%(ReferenceCopyLocalPaths.NuGetPackageId)' != 'Microsoft.CodeAnalysis.Common' | ||
| and '%(ReferenceCopyLocalPaths.NuGetPackageId)' != 'Microsoft.CodeAnalysis.Analyzers' | ||
| and !%(ReferenceCopyLocalPaths.NuGetPackageId.StartsWith('Microsoft.CodeAnalysis')) |
There was a problem hiding this comment.
The exclusion condition !%(ReferenceCopyLocalPaths.NuGetPackageId.StartsWith('Microsoft.CodeAnalysis')) is not valid MSBuild syntax for item metadata and will either fail evaluation or always evaluate truthy/falsey unexpectedly. That risks bundling Microsoft.CodeAnalysis.* assemblies into the analyzer package (causing bloat and potential host version conflicts). Use a proper MSBuild property function (e.g., $([System.String]::Copy('%(ReferenceCopyLocalPaths.NuGetPackageId)').StartsWith('Microsoft.CodeAnalysis'))) or an equivalent explicit check, and ensure the condition reliably filters all Microsoft.CodeAnalysis.* packages.
| and !%(ReferenceCopyLocalPaths.NuGetPackageId.StartsWith('Microsoft.CodeAnalysis')) | |
| and '$([System.String]::Copy('%(ReferenceCopyLocalPaths.NuGetPackageId)').StartsWith('Microsoft.CodeAnalysis'))' != 'True' |
| --> | ||
| <Target Name="_ScribeSdkAddAnalyzerDlls" BeforeTargets="_GetPackageFiles"> | ||
| <ItemGroup> | ||
| <None Include="$(OutputPath)$(AssemblyName).dll" |
There was a problem hiding this comment.
_ScribeSdkAddAnalyzerDlls packs $(OutputPath)$(AssemblyName).dll, which can point at the wrong location (e.g., multi-targeting adds a TFM subfolder, custom output paths, or when AssemblyName != TargetFileName). This can produce a NuGet package missing the analyzer DLL. Prefer packing $(TargetPath) (or $(TargetDir)$(TargetFileName)) so the correct built output is always included.
| <None Include="$(OutputPath)$(AssemblyName).dll" | |
| <None Include="$(TargetPath)" |
| <PropertyGroup> | ||
| <CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
CopyLocalLockFileAssemblies is being set in Sdk.targets, which means consumers cannot override it in their project file (late-phase properties win). However, the docs in this PR state that all properties can be overridden. Either move this default into Sdk.props with a conditional (so consumers can set it), or update the documentation to call out that dependency bundling is enforced by the SDK.
| | `CopyLocalLockFileAssemblies` | `true` | Enables private dependency bundling | | ||
|
|
||
| All properties can be overridden in your `.csproj`. | ||
|
|
||
| ## Stubs.cs Polyfills |
There was a problem hiding this comment.
This README says "All properties can be overridden in your .csproj", but the SDK sets/enforces some behavior in Sdk.targets (late phase) such as dependency bundling (CopyLocalLockFileAssemblies) and packaging targets, which cannot be overridden the same way. Please adjust the wording to distinguish overridable defaults (Sdk.props) from enforced behavior (Sdk.targets), and/or document the specific opt-out switches.
| All properties can be overridden in your `.csproj`. | ||
|
|
There was a problem hiding this comment.
This section says "All properties can be overridden in your .csproj", but the SDK also sets/enforces behavior in Sdk.targets (late phase) which consumers cannot override the same way (e.g., CopyLocalLockFileAssemblies as currently implemented, plus packaging targets). Please update the doc to clarify what is actually overridable vs enforced, and document the supported opt-outs.
| All properties can be overridden in your `.csproj`. | |
| Most of these values are SDK defaults and can be overridden in your `.csproj` by setting the property yourself. | |
| However, not every SDK behavior is overridable in the same way. Some behavior is applied or enforced later by the SDK's `.targets` files, so a property set in your project file may not win. In particular, private dependency bundling (`CopyLocalLockFileAssemblies` as configured by the SDK) and analyzer packaging are SDK-managed behaviors and should be treated as enforced unless the SDK documents a specific opt-out. | |
| ### Supported opt-outs | |
| The following opt-out is supported in the SDK configuration shown here: |
- Use $(TargetPath) instead of $(OutputPath)$(AssemblyName).dll for robustness - Fix MSBuild StartsWith syntax using proper property function - Move CopyLocalLockFileAssemblies to Sdk.props (overridable by consumers) - Clarify overridable defaults vs enforced packaging in docs Agent-Logs-Url: https://github.com/BulletsForHumanity/Scribe/sessions/ecc9d193-c204-49b1-8184-1d01b404e2b1 Co-authored-by: crazycrank <11194014+crazycrank@users.noreply.github.com>
StartsWithin dependency filtering$(TargetPath)instead of$(OutputPath)$(AssemblyName).dllfor analyzer DLL packagingCopyLocalLockFileAssembliesfrom Sdk.targets to Sdk.props (consumers can override)