Pipelines | Pre-compute all OneBranch package and file versions - #4652
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Stable test-feed publishing can collide, and the moved version-shape logic lacks required automated coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Centralizes OneBranch package/file version computation and corrects per-package SBOM metadata.
Changes:
- Removes revision-based versioning and standardizes SemVer stamping.
- Passes precomputed file versions through build and packaging jobs.
- Configures per-package SBOM identities and updates documentation/tests.
File summaries
| File | Review |
|---|---|
src/Microsoft.SqlServer.Server/Versions.props |
Standardizes SqlServer version generation. |
src/Microsoft.Data.SqlClient/Versions.props |
Critical: Stable test-feed versions can collide on repeated publishes; retain exact production versions while making test releases build-specific. |
eng/pipelines/onebranch/variables/package-variables.yml |
Removes obsolete versioning guidance. |
eng/pipelines/onebranch/steps/roslyn-analyzers-buildproj-step.yml |
Passes precomputed file versions to analysis builds. |
eng/pipelines/onebranch/steps/pack-buildproj-step.yml |
Passes file versions during packaging. |
eng/pipelines/onebranch/steps/build-buildproj-step.yml |
Passes file versions during builds. |
eng/pipelines/onebranch/stages/compute-versions-stage.yml |
Simplifies canonical version computation. |
eng/pipelines/onebranch/stages/build-stages.yml |
Distributes computed versions to package jobs. |
eng/pipelines/onebranch/sqlclient-official.yml |
Removes revision mode and fixes SBOM metadata. |
eng/pipelines/onebranch/sqlclient-non-official.yml |
Mirrors official-pipeline version and SBOM changes. |
eng/pipelines/onebranch/scripts/tests/compute-versions.Tests.ps1 |
Moderate: Add automated GetVersionsSqlClient and GetVersionsSqlServer coverage for stable/prerelease bases, with and without BuildSuffix. |
eng/pipelines/onebranch/scripts/compute-versions.ps1 |
Emits canonical package and file versions. |
eng/pipelines/onebranch/jobs/publish-symbols-job.yml |
Disables irrelevant SBOM generation. |
eng/pipelines/onebranch/jobs/build-buildproj-job.yml |
Sets package-specific SBOM metadata. |
build.proj |
Adds file-version forwarding parameters. |
.github/instructions/sqlclient-package-versions.instructions.md |
Documents the new version shapes. |
.github/instructions/onebranch-pipeline-design.instructions.md |
Documents per-job SBOM configuration. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4652 +/- ##
==========================================
- Coverage 71.88% 64.00% -7.89%
==========================================
Files 290 284 -6
Lines 45056 68006 +22950
==========================================
+ Hits 32389 43527 +11138
- Misses 12667 24479 +11812
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a73ce5e to
06f14b6
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Pipeline-wide versioning changes require human approval, and outdated version examples remain unresolved.
Review details
Suppressed comments (2)
build.proj:519
- The usage example immediately above this target is now outdated: with
BuildNumber=1234, the updatedVersions.propsappends the build number to the prerelease package version, so the shown7.1.0-preview1output is no longer possible. Update the example to reflect the new target output.
<_Cmd>"$(DotnetPath)dotnet" build "$(SqlClientProjectPath)" -getProperty:SqlClientPackageVersion $(BuildNumberArgument) $(BuildSuffixArgument) $(SqlClientNextVersionArgument)</_Cmd>
src/Microsoft.Data.SqlClient/Versions.props:78
- The new dot-separated prerelease shape is not reflected in the public build documentation.
BUILDGUIDE.md:302-308still saysBuildSuffix=ciproduces1.2.3-ci1234, while this line now produces1.2.3-ci.1234;build.proj:511-514also still shows a prerelease version without the appended build number. Please update these canonical examples with the behavior change so contributors can predict the generated artifact names.
<SqlClientPackageVersion Condition="$(_SqlClientCandidateVersion.Contains('-'))">$(_SqlClientCandidateVersion).$(BuildNumber)</SqlClientPackageVersion>
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Explicit four-component package-version overrides currently generate invalid five-component file versions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The new build and pack wrapper forwarding lacks direct regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/Versions.props:79
- The new separator leaves the repository's versioning documentation inconsistent with the implementation:
BUILDGUIDE.md:308still documents1.2.3-ci1234,Directory.Packages.props:46usespreview1-ci123, and.github/instructions/ado-pipelines.instructions.md:108describes a different old ordering. Since this changes every PR/CI package version, update those references to the new-ci.<BuildNumber>/-pr.<BuildNumber>shape in this PR.
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Balanced
priyankatiwari08
left a comment
There was a problem hiding this comment.
Version-shape consolidation looks correct; Versions.props global-property override makes the pre-computed file version win as intended. Two things:
globalSdl.sbomnow indirects through$(sbomPackageName)/$(sbomPackageVersion), but onlybuild-buildproj-job.ymlsets them.publish-nuget-package-job.ymlsetsob_outputDirectory: $(JOB_OUTPUT)and neither defines those variables nor setsob_sdl_sbom_enabled: false, so its SBOM will carry the literal unresolved macros.validate-signed-package-job.ymlhas the same gap.FileVersionSqlClient/FileVersionSqlServerare now pipeline-supplied inputs but only rejected when they have 5+ numeric components — see inline.
There was a problem hiding this comment.
🟡 Changes recommended
Moderate file-version compatibility and target-scope documentation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
build.proj:140
- This scope is broader than the implementation:
FileVersionSqlClientArgumentis forwarded only by the SqlClient-family Build and Pack targets, not by the Test targets included inPackageVersionSqlClient's scope above. Document the actual target set so callers do not expect the wrapper to affect test-triggered builds.
FileVersionSqlClient
Applies to: The same targets as PackageVersionSqlClient.
build.proj:187
- This scope is broader than the implementation:
FileVersionSqlServerArgumentis forwarded only byBuildSqlServerandPackSqlServer, whilePackageVersionSqlServeralso applies to SqlClient consumer and test targets. Document the actual target set so callers do not expect this wrapper to stamp those targets.
FileVersionSqlServer
Applies to: The same targets as PackageVersionSqlServer.
src/Microsoft.Data.SqlClient/Versions.props:112
- The new check validates component count but not the required 16-bit range:
1.2.3.65536matches this regex even thoughsrc/Directory.Build.props:116-120documents the file-version component as unsigned 16-bit. Since source builds treat warnings as errors, that input bypasses this diagnostic and fails later with the less useful compiler error. Validate each component's range as well and cover an out-of-range value in the tests.
Condition="!$([System.Text.RegularExpressions.Regex]::IsMatch('$(SqlClientFileVersion)', '^\d+\.\d+\.\d+\.\d+$'))"
src/Microsoft.Data.SqlClient/Versions.props:78
- The new dotted prerelease shape is implemented here, but contributor-facing examples still advertise the removed fused form:
BUILDGUIDE.md:308shows1.2.3-ci1234, andDirectory.Packages.props:46usespreview1-ci123. Update those examples to include the separator so local and CI consumers follow the canonical format.
<SqlClientPackageVersion Condition="$(_SqlClientCandidateVersion.Contains('-'))">$(_SqlClientCandidateVersion).$(BuildNumber)</SqlClientPackageVersion>
src/Microsoft.SqlServer.Server/Versions.props:108
- The new check validates component count but not the required 16-bit range:
1.2.3.65536matches this regex even thoughsrc/Directory.Build.props:116-120documents the file-version component as unsigned 16-bit. Since source builds treat warnings as errors, that input bypasses this diagnostic and fails later with the less useful compiler error. Validate each component's range as well and cover an out-of-range value in the tests.
Condition="!$([System.Text.RegularExpressions.Regex]::IsMatch('$(SqlServerFileVersion)', '^\d+\.\d+\.\d+\.\d+$'))"
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Review effort level: Balanced
93f2021
fb2f5d9 to
93f2021
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Version examples and build.proj option-scope documentation must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
build.proj:187
- This does not actually apply to all targets listed for
PackageVersionSqlServer: the new argument is forwarded only byBuildSqlServerandPackSqlServer, not by SqlClient build/pack or test targets that consume the package-version override. Document the actual scope so callers do not rely on an ignored option.
Applies to: The same targets as PackageVersionSqlServer.
build.proj:140
- The stated scope is broader than the implementation:
PackageVersionSqlClientapplies to severalTest*targets, butFileVersionSqlClientArgumentis only forwarded by the SqlClient-family build and pack targets. Since this option is intended for build-job stamping, narrow the documented scope rather than promising that test targets consume it.
Applies to: The same targets as PackageVersionSqlClient.
- Files reviewed: 20/20 changed files
- Comments generated: 1
- Review effort level: Balanced
Make the compute-versions stage the single source of every version the OneBranch build jobs consume, so nothing is re-derived downstream. - Remove the addRevision mode entirely. Package versions now have a single shape driven by the pipeline build number, and the 16-bit revision wrapping, the four-part package base handling, and the Build.BuildId plumbing are gone. - Move package version stamping out of PowerShell and into Versions.props. BuildSuffix now does what it always documented: it turns a stable base into a prerelease. Any version carrying a prerelease tag, from either source, is stamped with the build number; released versions are left untouched. - Publish SqlClient and SqlServer file versions from the compute-versions stage and pass them into the build jobs, which previously received a raw build number and re-derived the file version through MSBuild. build.proj gains opt-in FileVersion* arguments, so PR/CI and local builds are unchanged. - Fix SBOM metadata, which reported the pipeline run number as the version of a single hardcoded package name. Each build job now supplies the name and computed version of the package it produces, and jobs that publish no packages disable SBOM generation instead.
93f2021 to
b79b45f
Compare
Makes the
compute_versionsstage the single source of every version the OneBranch build jobs consume, so nothing is re-derived downstream.Remove the
addRevisionmodePackage versions had two mutually exclusive shapes. The revision shape (
7.1.0.34430-preview1) is gone, along with the 16-bit wrapping ofBuild.BuildId, the four-part package base special case in bothVersions.propsfiles, and theaddRevisionqueue-time parameter.Worth noting the shape it removes was also a versioning hazard: a four-part core sorts above the stable release, so
7.1.0.34430-preview1outranked7.1.0on the feed, and a four-part core with a prerelease tag isn't valid SemVer 2.0.Move package version stamping into
Versions.propsBuildSuffixnow does what it always documented — it turns a stable base into a prerelease. Any version carrying a prerelease tag, from either the declared version or the suffix, is stamped with the build number; released versions are left exactly as declared.NextVersionBuildSuffix7.1.0-preview37.1.0-preview3.26238.37.1.07.1.07.1.0-preview3ci7.1.0-preview3-ci.26238.37.1.0ci7.1.0-ci.26238.37.1.0-preview3-dev-ci26238.3→-ci.26238.3), because the old suffix path fused them. This is a deliberate improvement: SemVer splits the prerelease on dots, so the build number now compares numerically instead of lexically (previously…ci9sorted above…ci10). No in-repo consumer string-matches the old form.Pass pre-computed file versions into the build jobs
Build jobs received a raw build number and re-derived the file version through MSBuild. They now receive the file version the
compute_versionsstage already computed.build.projgains opt-inFileVersionSqlClient/FileVersionSqlServerarguments, which emit nothing unless set — so PR/CI and local builds are unchanged.This also fixes a latent bug:
compute-versions.ps1invoked theGetVersions*targets with a different build number than the build jobs used, then discarded the result and re-derived it in PowerShell.Assembly version stays derived from the file version, as it's always
major.0.0.0.Fix SBOM metadata
globalSdl.sbomreported$(Build.BuildNumber)(e.g.26238.3) as the version of a single hardcoded package name — a version matching no package produced, and used a single name for six different packages. Now it expands the SDL template with macro-expansion variables$(...)whose values are defined by each package's build job, and then interpolated for SBOM use. We are now using consistent package name/version values in each package's SBOM.Testing