Make STS2 SQLite driver opt-in - #2805
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes the STS2 SQLite driver truly opt-in at the Bootstrap (composition root) level to prevent non-production SQLite dependencies and native assets from inflating the default/portable ServiceLayer publish outputs. It also updates tests and documentation to enforce and explain the new production-default vs local/test boundary, including building a separate SQLite-enabled E2E “subject” into isolated output paths.
Changes:
- Make
IncludeSts2SqliteDriverdefault tofalseand conditionally reference/compile the SQLite driver only when explicitly enabled. - Update STS2 E2E harness to support spawning an isolated SQLite-enabled ServiceLayer build, and add assertions that default subjects contain no SQLite assets.
- Update generated/component docs to reflect conditional references and clarify the SQLite driver’s non-production role.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/sts2/Microsoft.SqlTools.Sts2.UnitTests/Architecture/DependencyMatrixTests.cs | Adds an architecture guard test verifying SQLite is opt-in at the Bootstrap csproj level. |
| test/sts2/Microsoft.SqlTools.Sts2.E2ETests/StdioE2ETests.cs | Adds a test asserting default subject outputs contain no SQLite assets; updates SQLite E2E test to spawn a SQLite-enabled subject. |
| test/sts2/Microsoft.SqlTools.Sts2.E2ETests/ServiceProcessClient.cs | Adds includeSqlite support to locate/spawn either default or isolated SQLite-enabled subject output. |
| test/sts2/Microsoft.SqlTools.Sts2.E2ETests/Microsoft.SqlTools.Sts2.E2ETests.csproj | Adds an MSBuild target to build an isolated SQLite-enabled ServiceLayer subject into separate bin/obj paths. |
| src/sts2/Microsoft.SqlTools.Sts2.Testing/GeneratedDocs.cs | Enhances generated component docs to include MSBuild Condition for conditional project references. |
| src/sts2/Microsoft.SqlTools.Sts2.Drivers.Sqlite/Microsoft.SqlTools.Sts2.Drivers.Sqlite.csproj | Clarifies non-production/test positioning in project description and comments about dependency propagation. |
| src/sts2/Microsoft.SqlTools.Sts2.Drivers.Sqlite/COMPONENT.md | Updates role description and documents opt-in entry into the ServiceLayer graph. |
| src/sts2/Microsoft.SqlTools.Sts2.Bootstrap/Sts2Bootstrap.cs | Wraps SQLite driver registration behind STS2_INCLUDE_SQLITE_DRIVER and centralizes driver construction. |
| src/sts2/Microsoft.SqlTools.Sts2.Bootstrap/Microsoft.SqlTools.Sts2.Bootstrap.csproj | Introduces IncludeSts2SqliteDriver defaulting to false, adds conditional define, and conditions the SQLite ProjectReference. |
| src/sts2/Microsoft.SqlTools.Sts2.Bootstrap/COMPONENT.md | Documents conditional dependency on Drivers.Sqlite and opt-in guidance. |
| docs/sts2/COMPONENTS.md | Updates generated output to include conditional project-reference conditions and updated SQLite role text. |
| docs/sts2/CLIENT.md | Documents that the sample uses the SQLite test driver and how to opt in for local builds. |
| Directory.Build.props | Adds isolated output/intermediate paths for the SQLite-enabled E2E subject to avoid contaminating default outputs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
CI follow-up: I reproduced the NU1510 failure locally; it was deterministic, not transient. The E2E project's nested MSBuild calls inherited the outer TargetFramework=net10.0 global property from Cake, which forced transitive projects (including the normally net8.0 SqlCore project) to net10.0 and made their framework-provided PackageReferences appear redundant. Commit 8a2b6bb removes TargetFramework from those nested calls so each project keeps its declared target framework. Validation: the exact failing dotnet build now completes with 0 warnings/errors, the STS2 E2E suite passes 7/7, and Cake BuildTest succeeds end to end. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
test/sts2/Microsoft.SqlTools.Sts2.UnitTests/Architecture/DependencyMatrixTests.cs:112
Assert.Contains(..., includeProperty.Attribute("Condition")?.Value)will throw if the Condition attribute is missing (null) rather than producing a clear assertion failure. Capture and assert the attribute is present first so failures are easier to diagnose.
This issue also appears on line 120 of the same file.
XElement includeProperty = Assert.Single(
bootstrapProject.Descendants("IncludeSts2SqliteDriver"));
Assert.Equal("false", includeProperty.Value);
Assert.Contains("IncludeSts2SqliteDriver", includeProperty.Attribute("Condition")?.Value);
test/sts2/Microsoft.SqlTools.Sts2.E2ETests/StdioE2ETests.cs:63
- The file glob pattern "sqlite" is case-sensitive on some platforms (e.g., Linux). This test could miss SQLite-related assets such as "SQLitePCLRaw.*" and pass even when SQLite files are present. Prefer enumerating all files and doing a case-insensitive substring check.
public void DefaultSubjectExcludesSqliteAssets()
{
string serviceDirectory = Path.GetDirectoryName(ServiceProcessClient.LocateServiceDll())!;
Assert.Empty(Directory.EnumerateFiles(serviceDirectory, "*sqlite*", SearchOption.AllDirectories));
test/sts2/Microsoft.SqlTools.Sts2.UnitTests/Architecture/DependencyMatrixTests.cs:122
- Similarly,
Assert.Contains(..., condition)will throw if the ProjectReference Condition attribute is missing. Asserting the attribute exists first provides a clearer test failure and avoids an unexpected exception path.
string? condition = sqliteReference.Attribute("Condition")?.Value;
Assert.Contains("IncludeSts2SqliteDriver", condition);
Assert.Contains("true", condition);
Why
STS2's SQLite driver is a non-production local/test component (
Production=false), but the product Bootstrap project referenced it unconditionally. A portable (RID-less) publish therefore pulledMicrosoft.Data.Sqlite, SQLitePCLRaw, and nativee_sqlite3assets for every supported runtime into the production bundle. RID-specific publishes select one native asset, which is why their size stayed roughly flat while the portable archive jumped sharply.What changed
IncludeSts2SqliteDriverdefault tofalseat the product composition root.true.bin/objpaths so it cannot overwrite or contaminate the default product subject.This leaves the production
sqlclientdriver and STS2 activation behavior unchanged. Local experiments can opt in explicitly:Measured package impact
Release-mode publishes from this branch:
win-x64self-contained, uncompressedThe default outputs contain zero SQLite files or
.deps.jsonentries. The opt-in portable output contains 36 SQLite-related files, including 29 native assets spanning Android, browser, iOS, Linux, macOS, and Windows; the opt-inwin-x64output contains only the matchinge_sqlite3.dllplus the managed driver files.Validation
vscode-mssqldev/refactor: build passed; lint passed after normalizing three pre-existing CRLF-only files in the disposable validation clone; VS Code unit suite 5,785 passed / 13 skipped; online VSIX packaging passed.kburtram/perftest: build and typecheck passed; 323 tests passed / 15 optional central-store tests skipped because that endpoint was unavailable.vscode-mssqldev/refactor: 10/10 passed with the default graph and the identical 10/10 passed with the SQLite-opt-in graph. The matrix covered 100k narrow rows, 300-column rows, large cells, blob/XML, forced spill/copy, 100 result sets, spatial, vector, cancellation, and disposal.All cross-repository checks used isolated clones; the active refactor worktrees were not modified.