Fix SqlServer sibling reference handling#4259
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how the repo’s sibling Microsoft.SqlServer.Server dependency is referenced under ReferenceType (Project vs Package), so package-mode builds/restores can resolve consistent versions and projects that require internals can explicitly enforce project-mode.
Changes:
- Switched several test/sample projects from unconditional
Microsoft.SqlServer.Serverpackage references toProjectReference/PackageReferenceconditioned on$(ReferenceType). - Added a unit-test guard to explicitly reject
ReferenceType=Package. - Updated
build.proj/Directory.Packages.propsto flowSqlServerPackageVersionin package mode and to packMicrosoft.SqlServer.Serverinto the local packages feed.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj | Adds Microsoft.SqlServer.Server project reference and rejects ReferenceType=Package. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UDTs/Utf8String/Utf8String.csproj | Conditional project/package reference for Microsoft.SqlServer.Server. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UDTs/Shapes/Shapes.csproj | Conditional project/package reference for Microsoft.SqlServer.Server. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UDTs/Circle/Circle.csproj | Conditional project/package reference for Microsoft.SqlServer.Server (also normalizes file header). |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/UdtTest/UDTs/Address/Address.csproj | Conditional project/package reference for Microsoft.SqlServer.Server (also normalizes file header). |
| src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj | Centralizes conditional Microsoft.SqlServer.Server reference for manual tests. |
| src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.csproj | Removes unused Microsoft.SqlServer.Server package reference from ref build project. |
| src/Microsoft.Data.SqlClient/notsupported/Microsoft.Data.SqlClient.csproj | Uses project reference for Microsoft.SqlServer.Server in project mode and package ref in package mode. |
| src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj | Formatting-only tweak to conditional reference items. |
| doc/samples/Microsoft.Data.SqlClient.Samples.csproj | Makes Microsoft.SqlServer.Server a conditional project/package reference based on ReferenceType. |
| Directory.Packages.props | Moves Microsoft.SqlServer.Server version declaration under ReferenceType=Package and uses $(SqlServerPackageVersion). |
| build.proj | Imports Versions.props to set default package versions in package mode; fixes SqlServer version forwarding; packs Microsoft.SqlServer.Server to $(PackagesDir); adds package-mode pack dependencies to build targets. |
| --> | ||
| <Target Name="BuildSqlClientNotSupported"> | ||
| <PropertyGroup> | ||
| <BuildSqlClientNotSupportedDependsOn Condition="'$(ReferenceType.ToLower())' == 'package'">PackAbstractions</BuildSqlClientNotSupportedDependsOn> |
There was a problem hiding this comment.
@benrr101 - Do we want these Build* and Pack* targets to depend on the Pack targets of the siblings this target depends on?
I think I would like this to work without any previous targets run, and use default package versions:
dotnet build -t:BuildSqlClient -p:ReferenceType=Package
There was a problem hiding this comment.
The latest commits achieve the above. Package mode now automatically ensures that the NuGet packages of sibling dependencies are built and put into packages/.
2851620 to
775fa11
Compare
f4fcba7 to
d0492fe
Compare
benrr101
left a comment
There was a problem hiding this comment.
Blocking on:
- Package/project reference going in their own item group, as per existing pattern
- Revisiting the intention of including the version props file in build.proj and confusing the recalculation of versions.
| <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" /> | ||
| <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" /> | ||
| <PackageReference Include="Microsoft.SqlServer.Server" /> | ||
| <PackageReference Include="Microsoft.SqlServer.Server" Condition="'$(ReferenceType)' == 'Package'" /> |
There was a problem hiding this comment.
Please follow the above pattern above - put both package and project references (like this) in one item group.
| <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" /> | ||
| <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" /> | ||
| <PackageReference Include="Microsoft.SqlServer.Server" /> | ||
| <PackageReference Include="Microsoft.SqlServer.Server" Condition="'$(ReferenceType)' == 'Package'" /> |
There was a problem hiding this comment.
Remove this in favor of the package/project reference in a separate item group above
| <!-- Imports ========================================================= --> | ||
| <Import Project="src/Directory.Build.props" /> | ||
|
|
||
| <!-- Import each project's Versions.props to apply default version numbers in Package mode. --> |
There was a problem hiding this comment.
I don't think this will work as well as you think it will ... The parameters passed to build.proj are PackageVersion* while the parameters passed to the csproj (and props files) are *PackageVersion. This was a compromise between wanting neat and orderly parameters in build2.proj, and not obliterating the existing csproj parameters.
There was a problem hiding this comment.
Yep, this wasn't the correct approach. Added imports to Directory.Packages.props to ensure defaults are computed early enough in the build process to be applied correctly for Package builds.
| Example: 1.0.1-dev2345 | ||
| --> | ||
| <PackageVersionAbstractions Condition="'$(PackageVersionAbstractions)' == ''" /> | ||
| <PackageVersionAbstractions Condition="'$(PackageVersionAbstractions)' == ''">$(AbstractionsPackageVersion)</PackageVersionAbstractions> |
There was a problem hiding this comment.
This more or less confound the whole versioning process I laid out...
The intention was that each project would know what its versions should be, and any projects that depend on another project just references that project and gets the sibling version. Further, the intention is that package reference goes away, so the remaining scenario where a package version is needed goes away.
Doing things this way means .... we calculate the versions with only some of the parameters provided, then call dotnet with this version, then recalculate the version with the package version provided. I think it's confusing and I'm not sure what scenario its trying to solve.
There was a problem hiding this comment.
Correct - see above reply.
| $(ReferenceTypeArgument) | ||
| $(PackageVersionAbstractionsArgument) | ||
| $(PackageVersionLoggingArgument) | ||
| $(PackageVersionSqlServerArgument) |
There was a problem hiding this comment.
This isn't being added as a parameter at the top.
There was a problem hiding this comment.
This property is computed as expected now.
d0492fe to
88d1198
Compare
88d1198 to
8200659
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Microsoft.SqlServer.Server/Microsoft.SqlServer.Server.csproj:13
- Because this Versions.props import is now gated by $(SqlServerVersionsImported), any earlier import (e.g., via Directory.Packages.props in ReferenceType=Package builds) will prevent re-importing after src/Directory.Build.props defines FileVersionBuildNumber. If the earlier import occurs before FileVersionBuildNumber is set, SqlServerFileVersion/SqlServerAssemblyVersion can be computed incorrectly and then “locked in” for the rest of the build. To make the guard safe, ensure Versions.props can compute correct versions even when imported before Directory.Build.props (e.g., define BuildNumber/FileVersionBuildNumber defaults inside Versions.props).
<!-- Versioning ====================================================== -->
<Import Project="Versions.props" Condition="'$(SqlServerVersionsImported)' != 'true'" />
<PropertyGroup>
<AssemblyVersion>$(SqlServerAssemblyVersion)</AssemblyVersion>
<FileVersion>$(SqlServerFileVersion)</FileVersion>
<Version>$(SqlServerPackageVersion)</Version>
…mbinations to help catch obvious problems pre-commit. - Updated CodeQL to use the new BuildAll target, since it compiles more code than using the solution. - Removed some obsolete and spurious <Project> attributes. - Reduced build console noise about repo URLs.
| if: matrix.build-mode == 'manual' | ||
| shell: bash | ||
| run: dotnet build src/Microsoft.Data.SqlClient.slnx | ||
| run: dotnet build build.proj -t:BuildAll |
There was a problem hiding this comment.
This compiles more code than the bare solution due to our TFM choice via host OS conditionals in some projects.
|
|
||
| <!-- Versioning ====================================================== --> | ||
| <Import Project="Versions.props" /> | ||
| <Import Project="Versions.props" Condition="'$(AkvProviderVersionsImported)' != 'true'" /> |
There was a problem hiding this comment.
See Directory.Packages.props for an explanation of why Versions.props may already be imported by the time we get here. Same for all the other driver projects.
| <Target Name="TrimDocsForIntelliSense" | ||
| AfterTargets="Build" | ||
| Condition="'$(IsCrossTargetingBuild)' != 'true' AND '$(GenerateDocumentationFile)' == 'true'"> | ||
| Condition="'$(TrimDocs)' != 'false' AND '$(IsCrossTargetingBuild)' != 'true' AND '$(GenerateDocumentationFile)' == 'true'"> |
There was a problem hiding this comment.
The new BuildAll target wants to generate the docs, but doesn't want to run the trimming.
| <NuspecProperties>$(NuspecProperties);COMMITID=$(CommitId)</NuspecProperties> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- Check for empty sibling package versions. --> |
There was a problem hiding this comment.
@benrr101 - Do you think we need these checks? AI suggested them, but then why not for the other driver projects that depend on siblings?
| namespace Microsoft.Data.SqlClient.TestCommon; | ||
|
|
||
| /// <summary> | ||
| /// Provides a shared xUnit conditional check for tests that depend on SQL Server UDT types. |
There was a problem hiding this comment.
This is a side-effect of promoting the SqlServer project to a first-class citizen in the build system. Now that we have proper Project vs Package refs for it throughout, we end up sometimes wanting to run tests with an unsigned SqlServer assembly, which we never did before. A few of our tests require the unrelated Microsoft.SqlServer.Types package, which in turn depends on SqlServer, and on .NET Framework will require a signed SqlServer assembly.
We now avoid running these few tests when SqlServer isn't signed and we're using .NET Framework. These tests still run fine on all .NET runtimes. Our ADO.Net CI pipelines will sign everything, and these tests will run for .NET Framework there.
Alternatively, we could isolate these tests into their own project that always references a known published SqlServer package that is signed, and then they could run all the time. Thoughts?
| @@ -1,5 +1,6 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project ToolsVersion="Current" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
There was a problem hiding this comment.
I think previously Build was the default because it appeared first. Now it's official.
- Extract BuildNumber/FileVersionBuildNumber into src/Versioning.props so that FileVersionBuildNumber is defined before any Versions.props runs, regardless of Package vs Project mode. - Import Versioning.props from Directory.Packages.props (Package mode) and Directory.Build.props (Project mode, with guard). - Remove unnecessary blank line in Directory.Packages.props. - Fix 'Setup of list' comment wording in build.proj. - Add 'AKV Provider has no tests' note in build.proj BuildTests target.
Directory.Build.props evaluates before Directory.Packages.props, so import Versioning.props unconditionally from Directory.Build.props only. Remove the import from Directory.Packages.props and the now-unnecessary guard flag.
Only one import site exists (Directory.Build.props evaluates before Directory.Packages.props), so a separate file is unnecessary.
- Replace hardcoded SqlServer.Server 1.0.0 dependency in nuspec with $SqlServerPackageVersion$ token, matching Abstractions/Logging pattern. - Import SqlServer Versions.props in csproj for version resolution during pack. - Add SqlServerPackageVersion validation and token expansion in PrepareSqlClientPackNuspec target. - Pass PackageVersionSqlServer through build.proj PackSqlClient target. - Fix BuildTests to not pass ReferenceType to UnitTests (project-only).
Protect against spaces in checkout paths by quoting all project path properties in Exec commands, matching the pattern used by other targets.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
build.proj:459
- The
dotnet buildinvocation uses the unquoted$(SqlClientRefProjectPath). This breaks when the repo is checked out under a path containing spaces; other targets in this file quote project paths. Quote the project path consistently here (and keep the rest of the command unchanged).
<DotnetCommand>
"$(DotnetPath)dotnet" build $(SqlClientRefProjectPath)
-p:Configuration=$(Configuration)
$(SigningKeyPathArgument)
build.proj:489
- The
dotnet buildinvocation uses the unquoted$(SqlClientProjectPath). If the repo path contains spaces, this command line will fail to parse correctly. Quote$(SqlClientProjectPath)(matching the quoting used in other build/pack commands in this file).
This issue also appears on line 515 of the same file.
<PropertyGroup>
<DotnetCommand>
"$(DotnetPath)dotnet" build $(SqlClientProjectPath)
-p:Configuration=$(Configuration)
-p:TargetOs=Unix
$(SigningKeyPathArgument)
build.proj:520
- The
dotnet buildinvocation uses the unquoted$(SqlClientProjectPath). This can fail when the working directory path contains spaces; please quote the project path consistently with other targets (e.g., PackSqlClient and ancillary Build* targets).
<PropertyGroup>
<DotnetCommand>
"$(DotnetPath)dotnet" build $(SqlClientProjectPath)
-p:Configuration=$(Configuration)
-p:TargetOs=Windows_NT
$(SigningKeyPathArgument)
| public static class SqlServerStrongNameTestCondition | ||
| { | ||
| /// <summary> | ||
| /// Gets whether SQL Server UDT tests are safe to run in the current runtime/signing context. | ||
| /// </summary> | ||
| public static bool IsUnsignedSqlServerAssemblyUsable | ||
| { |
The Azure test jobs restore SqlClient as a NuGet package, which has a transitive dependency on Microsoft.SqlServer.Server. In Package mode the CI-versioned SqlServer package (e.g. 1.0.0-ci13309) only exists as a pipeline artifact, so we must download it to packages/ before restore. Add sqlServerArtifactsName parameter and DownloadPipelineArtifact step to test-azure-package-ci-job.yml, pass it through the stage template, and add build_sqlserver_package_stage to the Azure stage dependencies in ci-core.
Add sqlServerPackageVersion and sqlServerArtifactsName parameters to every pipeline layer that already forwarded Abstractions/Logging versions but was missing SqlServer: Pipeline build/pack path: - ci-project-build-step.yml: forward to Build Driver and AKV Provider - ci-build-nugets-job.yml: add param, download artifact, forward to build steps and PackSqlClient - build-sqlclient-package-ci-stage.yml: add params, forward to job - dotnet-sqlclient-ci-core.yml: pass to MDS stage, add build_sqlserver_package_stage dependency Pipeline test path: - run-all-tests-step.yml: add param, pass -p:PackageVersionSqlServer to all 14 MSBuild test invocations - ci-run-tests-job.yml: add params, download SqlServer artifact, forward to run-all-tests-step - ci-run-tests-stage.yml: add params, forward to both job invocations - dotnet-sqlclient-ci-core.yml: pass to test stage, add build_sqlserver_package_stage dependency build.proj targets: - TestSqlClientFunctional, TestSqlClientManual, BuildAkvProvider, PackAkvProvider, TestAzure: add $(PackageVersionSqlServerArgument)
| /// Returns true when an unsigned SqlServer assembly should be usable in conjunction with other | ||
| /// assemblies that explicitly depend on a strongly-named SqlServer assembly. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Why this exists: in local builds and PR pipeline runs, the <c>Microsoft.SqlServer.Server</c> | ||
| /// assembly is likely to be produced unsigned. Some UDT tests reference | ||
| /// <c>Microsoft.SqlServer.Types</c> (a publicly published NuGet package, not owned by us), | ||
| /// which requires a strongly named <c>Microsoft.SqlServer.Server</c> assembly. The .NET | ||
| /// runtime doesn't enforce this relationship, and the tests run without incident regardless of | ||
| /// the signed-ness of the SqlServer assembly. However, .NET Framework _does_ enforce that the | ||
| /// SqlServer assembly us signed, and the tests fail to compile and/or run. | ||
| /// | ||
| /// This situation can occur in both Project and Package based test runs, the latter when the | ||
| /// consumed SqlServer package was produced from an unsigned assembly. | ||
| /// | ||
| /// What this checks: on .NET Framework, it loads the UDT attribute type from | ||
| /// <c>Microsoft.SqlServer.Server</c> and verifies that the assembly has a non-empty public key | ||
| /// token (is strongly named). On .NET, this always returns <see langword="true"/> because | ||
| /// runtime strong-name validation is not enforced the same way. |
| /// which requires a strongly named <c>Microsoft.SqlServer.Server</c> assembly. The .NET | ||
| /// runtime doesn't enforce this relationship, and the tests run without incident regardless of | ||
| /// the signed-ness of the SqlServer assembly. However, .NET Framework _does_ enforce that the | ||
| /// SqlServer assembly us signed, and the tests fail to compile and/or run. |
Summary
Core fix
Microsoft.SqlServer.Serversibling references across affected projects to respectReferenceType(Project vs Package)SqlServerPackageVersionwiring through build orchestrationAdditional changes
Microsoft.SqlServer.Serverpack output path so artefacts land in the expectedartifacts/directorySqlServerStrongNameTestConditionhelper to avoidFileLoadException(0x80131044) when the assembly is unsigned (project-ref or local-feed builds)Versions.propsimportsSqlServerPackageVersiontodoc/samplesprojectMicrosoft.Data.SqlClient.TestCommonto simplify shared test helpersBuildAlltarget tobuild.projthat builds all projects across all OS/TFM combinations; hook CodeQL toBuildAllso more code is compiled during analysis<Project>attributes (ToolsVersion, strayDefaultTargets) from props filesBUILDGUIDE.mdaligned with current build/pack entrypoint guidanceMicrosoft.Data.SqlClient.csprojMicrosoft.SqlServer.Serverdependency version in SqlClient nuspec (was hardcoded1.0.0, now uses$SqlServerPackageVersion$matching the Abstractions/Logging pattern)Versions.propsduring pack for dynamic version resolution; add validation and token expansion inPrepareSqlClientPackNuspec$(PackageVersionSqlServerArgument)throughPackSqlClienttarget inbuild.projBuildTeststo not forwardReferenceTypeto UnitTests (project-only)BuildTests/BuildSamples/BuildToolsExec commands for paths-with-spaces safetyBuildTestsValidation
dotnet build build.proj -p:Configuration=Debug— 0 warnings, 0 errorsdotnet build build.proj -t:Build -p:Configuration=Debug -p:ReferenceType=Package— 0 warnings, 0 errorsdotnet build build.proj -t:BuildAll -p:Configuration=Debug— 0 warnings, 0 errorsdotnet build build.proj -t:PackSqlClient -p:Configuration=Debug— SqlServer.Server dependency now1.0.0-dev(dynamic)dotnet build src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj -f net8.0 -v minimaldotnet build src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj -f net8.0 -v minimaldotnet build build.proj -t:BuildSqlClientUnix -p:ReferenceType=Package -v minimaldotnet build src/Microsoft.Data.SqlClient.slnx -v minimal