Skip to content

Fix SqlServer sibling reference handling#4259

Draft
paulmedynski wants to merge 9 commits into
mainfrom
dev/paul/reference-type
Draft

Fix SqlServer sibling reference handling#4259
paulmedynski wants to merge 9 commits into
mainfrom
dev/paul/reference-type

Conversation

@paulmedynski
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski commented May 4, 2026

Summary

  • Align Microsoft.SqlServer.Server sibling references across affected projects to respect ReferenceType (Project vs Package)
  • Ensure package-mode version flow includes SqlServerPackageVersion wiring through build orchestration
  • Update test/sample project references for consistent restore/build behavior
  • Keep BUILDGUIDE.md aligned with the current build/pack entrypoint guidance
  • Include post-rebase conflict cleanup in Microsoft.Data.SqlClient.csproj

Validation

  • dotnet build src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj -f net8.0 -v minimal
  • dotnet build src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj -f net8.0 -v minimal
  • dotnet build build.proj -t:BuildSqlClientUnix -p:ReferenceType=Package -v minimal
  • dotnet build src/Microsoft.Data.SqlClient.slnx -v minimal
  • dotnet build build.proj -p:Configuration=Debug

Copilot AI review requested due to automatic review settings May 4, 2026 18:27
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board May 4, 2026
@paulmedynski paulmedynski added the Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems. label May 4, 2026
@paulmedynski paulmedynski moved this from To triage to In progress in SqlClient Board May 4, 2026
@paulmedynski paulmedynski added this to the 7.1.0-preview2 milestone May 4, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Server package references to ProjectReference/PackageReference conditioned on $(ReferenceType).
  • Added a unit-test guard to explicitly reject ReferenceType=Package.
  • Updated build.proj/Directory.Packages.props to flow SqlServerPackageVersion in package mode and to pack Microsoft.SqlServer.Server into 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.

Comment thread build.proj Outdated
Comment thread doc/samples/Microsoft.Data.SqlClient.Samples.csproj Outdated
Comment thread src/Microsoft.Data.SqlClient/notsupported/Microsoft.Data.SqlClient.csproj Outdated
Comment thread build.proj Outdated
Comment thread build.proj Outdated
Comment thread build.proj
Comment thread build.proj Outdated
-->
<Target Name="BuildSqlClientNotSupported">
<PropertyGroup>
<BuildSqlClientNotSupportedDependsOn Condition="'$(ReferenceType.ToLower())' == 'package'">PackAbstractions</BuildSqlClientNotSupportedDependsOn>
Copy link
Copy Markdown
Contributor Author

@paulmedynski paulmedynski May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest commits achieve the above. Package mode now automatically ensures that the NuGet packages of sibling dependencies are built and put into packages/.

@paulmedynski paulmedynski force-pushed the dev/paul/dotnet-pack-sqlclient branch from 2851620 to 775fa11 Compare May 5, 2026 12:54
Copilot AI review requested due to automatic review settings May 5, 2026 13:14
@paulmedynski paulmedynski force-pushed the dev/paul/reference-type branch from f4fcba7 to d0492fe Compare May 5, 2026 13:14
@paulmedynski paulmedynski changed the title Fix SqlServer sibling reference handling Workstream 1.5: SqlClient dotnet pack migration with sibling reference alignment May 5, 2026
@paulmedynski paulmedynski changed the title Workstream 1.5: SqlClient dotnet pack migration with sibling reference alignment Sibling prokect reference alignment May 5, 2026
@paulmedynski paulmedynski changed the title Sibling prokect reference alignment Sibling project reference alignment May 5, 2026
@paulmedynski paulmedynski changed the title Sibling project reference alignment Fix SqlServer sibling reference handling May 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Comment thread BUILDGUIDE.md Outdated
Comment thread Directory.Packages.props Outdated
Copy link
Copy Markdown
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the above pattern above - put both package and project references (like this) in one item group.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" />
<PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" />
<PackageReference Include="Microsoft.SqlServer.Server" />
<PackageReference Include="Microsoft.SqlServer.Server" Condition="'$(ReferenceType)' == 'Package'" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this in favor of the package/project reference in a separate item group above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread build.proj Outdated
<!-- Imports ========================================================= -->
<Import Project="src/Directory.Build.props" />

<!-- Import each project's Versions.props to apply default version numbers in Package mode. -->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread build.proj Outdated
Example: 1.0.1-dev2345
-->
<PackageVersionAbstractions Condition="'$(PackageVersionAbstractions)' == ''" />
<PackageVersionAbstractions Condition="'$(PackageVersionAbstractions)' == ''">$(AbstractionsPackageVersion)</PackageVersionAbstractions>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - see above reply.

Comment thread build.proj
$(ReferenceTypeArgument)
$(PackageVersionAbstractionsArgument)
$(PackageVersionLoggingArgument)
$(PackageVersionSqlServerArgument)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't being added as a parameter at the top.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is computed as expected now.

@paulmedynski paulmedynski force-pushed the dev/paul/reference-type branch from d0492fe to 88d1198 Compare May 8, 2026 13:32
Base automatically changed from dev/paul/dotnet-pack-sqlclient to main May 8, 2026 19:10
Copilot AI review requested due to automatic review settings May 8, 2026 20:03
@paulmedynski paulmedynski force-pushed the dev/paul/reference-type branch from 88d1198 to 8200659 Compare May 8, 2026 20:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj Outdated
Comment thread build.proj
Comment thread build.proj Outdated
Comment thread build.proj
Copilot AI review requested due to automatic review settings May 11, 2026 16:05
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.

Comment thread src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj Outdated
Comment thread build.proj
Comment thread src/Microsoft.Data.SqlClient/notsupported/Microsoft.Data.SqlClient.csproj Outdated
Copilot AI review requested due to automatic review settings May 11, 2026 18:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Comment thread Directory.Packages.props
Comment on lines +4 to +8
<!-- Import Versions.props files when building via Package references -->
<!-- This makes version properties available early for package pinning -->
<ImportGroup Condition="'$(ReferenceType)' == 'Package'">
<Import Project="src/Microsoft.Data.SqlClient/Versions.props" />
<Import Project="src/Microsoft.SqlServer.Server/Versions.props" />
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems.

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants