Migrate SqlClient packaging to dotnet pack and centralize pack fallback#4257
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates Microsoft.Data.SqlClient packaging from nuget.exe pack to an SDK-driven dotnet pack flow, moving pack configuration into the main project and introducing an intermediate nuspec generation step to avoid SDK/NuGet token-substitution issues for dependency versions.
Changes:
- Switch
PackSqlClientfromnuget.exe packtodotnet packinbuild.proj. - Add
dotnet pack/nuspec configuration toMicrosoft.Data.SqlClient.csproj, includingPrepareSqlClientPackNuspecto materialize dependency versions + package version into an intermediate nuspec. - Add documentation describing the pack flow and the upstream SDK substitution behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/sqlclient-dotnet-pack.md | New doc describing the SDK-based pack flow, repro, and rationale for generating an intermediate nuspec. |
| src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj | Adds pack defaults and a pre-GenerateNuspec target to generate a token-expanded nuspec for dotnet pack. |
| build.proj | Updates PackSqlClient target to run dotnet pack and removes NugetPath/nuget.exe usage. |
Comments suppressed due to low confidence (1)
build.proj:545
GetSqlClientPackageVersionCommandusesdotnet build ... -getProperty:SqlClientPackageVersion.-getPropertyis an MSBuild switch and may be rejected bydotnet buildargument parsing (unless forwarded via--), which would breakPackSqlClient. Consider switching back todotnet msbuildfor this query, or explicitly forwarding MSBuild-only args (dotnet build ... -- -getProperty:...).
<GetSqlClientPackageVersionCommand>
"$(DotnetPath)dotnet" build "$(SqlClientProjectPath)"
-nologo
-verbosity:quiet
-getProperty:SqlClientPackageVersion
|
Addressing Copilot review comments from commit 2851620: Comment on line 116 (template brittleness): Replaced hardcoded XML string replacement with dedicated Comment on line 47 (repro command docs): Added clarifying text explaining the repro command demonstrates the SDK bug. Added a second example with the required -p:PackageVersionAbstractions and -p:PackageVersionLogging MSBuild properties that triggers PrepareSqlClientPackNuspec. Comment on line 51 (spelling): Fixed 'occured' → 'occurred'. Comment in BUILDGUIDE.md (grammar): Fixed 'bulding' → 'building' and 'also handle by' → 'also handled by'. Comment on line 80 (NuspecVersion): Clarified that NuspecVersion is derived from $(Version) by SDK defaults and removed it from the dynamic values list. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4257 +/- ##
==========================================
- Coverage 65.95% 64.39% -1.56%
==========================================
Files 276 270 -6
Lines 42981 65777 +22796
==========================================
+ Hits 28346 42357 +14011
- Misses 14635 23420 +8785
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace nuget.exe invocation in build.proj with dotnet pack. Add PrepareSqlClientPackNuspec target in the SqlClient csproj to materialize dependency version tokens into an intermediate nuspec before pack runs, working around SDK token-substitution limitations in dependency version fields (dotnet/sdk#15482). Pack design notes and known SDK behavior documented in src/Microsoft.Data.SqlClient/src/sqlclient-dotnet-pack.md.
…, and NuspecVersion clarity - Replace hardcoded version XML with $NuspecVersion$ token in nuspec template (fixes brittle string replacement) - Update csproj to replace the dedicated token instead of hardcoded XML string (resilient to formatting changes) - Add clarifying text to repro command explaining PrepareSqlClientPackNuspec requirements - Fix spelling: 'occured' → 'occurred' in error message - Fix grammar/spelling in BUILDGUIDE.md: 'bulding' → 'building', 'also handle by' → 'also handled by' - Clarify NuspecVersion derives from $(Version) by SDK defaults and doesn't need explicit passing
- Align PrepareSqlClientPackNuspec error guidance with direct dotnet pack properties and build.proj aliases - Update sqlclient-dotnet-pack.md replacement list to use $NuspecVersion$ token - Correct direct repro command to pass AbstractionsPackageVersion/LoggingPackageVersion and clarify entrypoint property translation
apoorvdeshmukh
left a comment
There was a problem hiding this comment.
LGTM overall. Branch has some expected conflicts that need to be resolved and copilot has some doc comments that can be considered.
- Move Abstractions/Logging version fallback into SqlClient pack target - Reuse shared sibling project paths and DOTNET_HOST_PATH - Document intermediate nuspec generation and sibling version evaluation - Pass NuspecVersion through shared nuget pack pipeline properties
benrr101
left a comment
There was a problem hiding this comment.
I guess it's good 🤷♂️
I'm a bit sketched out by all the magic that's going on with known-but-unknown build targets, known-but-unknown properties, etc. I almost think its better to just keep using nuget.exe since it's clearer what's going on rather than shoehorning the behavior into dotnet pack. But it is nice to not have to have a separate nuget.exe file to do everything.
The "right" way to do this is to eliminate os-specific builds, eliminate the ref and unsupported projects, and then eliminate the nuspec file. But ... that's a lot longer of a process, and idk if we'll ever get funding for it. Anyways, just know I'm uneasy about this change but I'll go ahead with it.
Summary
PackSqlClientfromnuget.exe packtodotnet packsrc/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj$Version$are materialized before SDK pack runsbuild.projto precompute thembuild.projPackSqlClientso it forwards pack inputs and lets the SqlClient project own SqlClient-specific pack behaviornuget packpipeline template to passVersionvia-propertiesBUILDGUIDE.md,TESTGUIDE.md, andsrc/Microsoft.Data.SqlClient/src/sqlclient-dotnet-pack.md, and address the Copilot review follow-ups for property naming, repro docs, and nuspec token detailsValidation
dotnet msbuild build.proj -t:BuildSqlClientdotnet msbuild build.proj -t:PackSqlClient -p:PackBuild=falsemainusingnuget.exe packagainst the same built artifacts for comparison.nupkgfiles after extractionrepository commit,lastModifiedBy, and derived relationship/core-properties identifiers)dotnet msbuild src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj -t:PrepareSqlClientPackNuspec -p:ReferenceType=Project -p:BuildNumber=1234 -p:BuildSuffix=dev -p:SqlClientPackageVersion=7.1.0-preview1-dev1234Notes
main