Conversation
…blish-symbols-step.
benrr101
added
the
Common Project 🚮
There was a problem hiding this comment.
Pull request overview
Updates OneBranch official/non-official pipeline templates to build Microsoft.Data.SqlClient (MDS) from build2.proj (common project) and refactors pipeline steps by removing the prior “compound-*” templates in favor of more granular build/sign/pack/analyze steps.
Changes:
- Replace the old SqlClient official build job with a new
build-signed-mds-package-job.ymlthat drives MDS build/pack viabuild2.proj. - Add new OneBranch step templates for Roslyn analyzers, MDS build/pack, ESRP signing, APIScan file extraction, and symbols publishing parameterization.
- Remove deprecated “compound-*” step templates and the prior SqlClient job template.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient.sln | Updates solution items to reference new OneBranch job/step templates. |
| eng/pipelines/onebranch/variables/common-variables.yml | Updates comments around template aliases. |
| eng/pipelines/onebranch/stages/build-stages.yml | Switches SqlClient build job to new MDS/build2-based job and wires new parameters. |
| eng/pipelines/onebranch/jobs/build-signed-mds-package-job.yml | New job orchestrating build2-based MDS build/analyze/sign/pack/publish-symbols. |
| eng/pipelines/onebranch/jobs/build-signed-csproj-package-job.yml | Migrates to new Roslyn analyzers/sign/pack/symbol templates. |
| eng/pipelines/onebranch/steps/build-mds-step.yml | New step template to run build2.proj -t:BuildMds. |
| eng/pipelines/onebranch/steps/pack-mds-step.yml | New step template to run build2.proj -t:PackMds. |
| eng/pipelines/onebranch/steps/extract-apiscan-files-mds-step.yml | New step template to copy MDS DLL/PDB outputs to APIScan folders. |
| eng/pipelines/onebranch/steps/roslyn-analyzers-mds-step.yml | New step template to run Roslyn analyzers for build2-based MDS build. |
| eng/pipelines/onebranch/steps/roslyn-analyzers-csproj-step.yml | New generic Roslyn analyzers step for build.proj-driven builds. |
| eng/pipelines/onebranch/steps/publish-symbols-step.yml | Refactors the symbols publish template parameter model and implementation. |
| eng/pipelines/onebranch/steps/esrp-dll-signing-step.yml | Updates ESRP DLL signing inputs/paths and parameter quoting. |
| eng/pipelines/onebranch/steps/esrp-nuget-signing-step.yml | New dedicated ESRP NuGet signing step template. |
| eng/pipelines/onebranch/steps/pack-csproj-step.yml | New dedicated pack step for csproj-based extension packages. |
| eng/pipelines/onebranch/steps/build-csproj-step.yml | Comment update to reference new pack step template name. |
| eng/pipelines/onebranch/steps/esrp-code-signing-step.yml | Removed legacy combined DLL/pkg signing template. |
| eng/pipelines/onebranch/steps/compound-publish-symbols-step.yml | Removed deprecated compound symbols template. |
| eng/pipelines/onebranch/steps/compound-nuget-pack-step.yml | Removed deprecated compound NuGet pack template. |
| eng/pipelines/onebranch/steps/build-all-configurations-signed-dlls-step.yml | Removed legacy MDS build-all-configurations step driven by build.proj. |
| eng/pipelines/onebranch/jobs/build-signed-sqlclient-package-job.yml | Removed prior SqlClient official build job template. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #4068 +/- ##
==========================================
- Coverage 73.23% 68.13% -5.11%
==========================================
Files 280 275 -5
Lines 43000 66924 +23924
==========================================
+ Hits 31491 45598 +14107
- Misses 11509 21326 +9817
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:
|
github-project-automation
bot
moved this from In review
to In progress
in SqlClient Board
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
Really just the one comment, otherwise looks good. I would also like to review the pipeline run when we get one that's green.
mdaigle
left a comment
mdaigle
left a comment
There was a problem hiding this comment.
Really just the one comment, otherwise looks good. I would also like to review the pipeline run when we get one that's green.
There was a problem hiding this comment.
🤖
Code Review Summary
Thanks for moving the MDS build to build2.proj — the cleanup of the "compound-" prefix and the separation of MDS into its own job/step templates is a nice improvement.
I found 3 critical issues and 1 major issue that will likely cause the official pipeline to produce incorrectly signed or broken packages:
Critical
| # | Issue | File |
|---|---|---|
| C1 | Typo: PackageVersionAbstraction → PackageVersionAbstractions (missing trailing 's') — pack step will fail |
pack-mds-step.yml:27 |
| C2 | ESRP NuGet signing scans wrong directory: Signing searches $(PACK_OUTPUT) (output/) but PackMds emits .nupkg to $(BUILD_OUTPUT)/Microsoft.Data.SqlClient/Package-Release/ — packages ship unsigned |
build-signed-mds-package-job.yml:142 |
| C3 | PackMds re-builds over signed DLLs: DependsOnTargets="BuildMds" re-triggers the build in a fresh MSBuild invocation, overwriting ESRP-signed DLLs. -p:PackBuild=false has no effect (not implemented in build2.proj) |
pack-mds-step.yml:25 |
Major
| # | Issue | File |
|---|---|---|
| M1 | $(PACKAGE_NAME) is undefined: Symbols publish with empty product name |
build-signed-mds-package-job.yml:157 |
Minor (not commented inline)
- Missing
@selfonpack-mds-step.ymltemplate reference (line 139 of the job file) ob_outputDirectory: '$(BUILD_OUTPUT)'publishes the entireartifacts/tree instead of just nupkg — compare with csproj jobs which use$(PACK_OUTPUT)
Question
Has the test build (pipeline 144204) successfully produced and validated a signed MDS package? The issues above should cause failures in the signing/validation stages.
| msbuildArguments: >- | ||
| -t:PackMds | ||
| -p:PackBuild=false | ||
| -p:PackageVersionAbstractions="${{ parameters.abstractionsPackageVersion }}" |
There was a problem hiding this comment.
Critical: Typo — PackageVersionAbstraction should be PackageVersionAbstractions
The MSBuild property in build2.proj is PackageVersionAbstractions (with trailing s). This typo means the PackMds target will see PackageVersionAbstractions as empty and will fail with:
Error: PackageVersionAbstractions must be set!
The build step (build-mds-step.yml) uses the correct spelling (PackageVersionAbstractions), so only the pack step is affected.
Fix: Change -p:PackageVersionAbstraction= to -p:PackageVersionAbstractions=
| abstractionsPackageVersion: '${{ parameters.abstractionsPackageVersion }}' | ||
| loggingPackageVersion: '${{ parameters.loggingPackageVersion }}' | ||
| mdsPackageVersion: '${{ parameters.mdsPackageVersion }}' | ||
|
|
There was a problem hiding this comment.
Critical: ESRP NuGet signing will not find any packages
The esrp-nuget-signing-step.yml scans $(PACK_OUTPUT) (= $(REPO_ROOT)/output) for *.*nupkg. However, the PackMds target in build2.proj outputs packages to:
$(MdsArtifactRoot)/$(ReferenceType)-$(Configuration)
= artifacts/Microsoft.Data.SqlClient/Package-Release/
This is under $(BUILD_OUTPUT) (artifacts/), not $(PACK_OUTPUT) (output/). The ESRP NuGet signing step will find zero packages and exit silently, resulting in unsigned NuGet packages being published as the pipeline artifact.
Fix options:
- Add a step before NuGet signing to copy the
.nupkg/.snupkgfromartifacts/Microsoft.Data.SqlClient/Package-Release/to$(PACK_OUTPUT), or - Modify the
PackMdstarget to output directly to$(PACK_OUTPUT)by passing-p:MdsArtifactRoot=$(PACK_OUTPUT)or an equivalent override
| solution: '$(REPO_ROOT)/build2.proj' | ||
| configuration: 'Release' | ||
| msbuildArguments: >- | ||
| -t:PackMds |
There was a problem hiding this comment.
Critical: PackMds will re-trigger BuildMds, overwriting ESRP-signed DLLs
The pipeline flow is: Build → ESRP DLL Sign → Pack → ESRP NuGet Sign.
However, PackMds in build2.proj has DependsOnTargets="BuildMds". Since the build step and pack step are separate MSBuild@1 task invocations, MSBuild's "run each target once" guarantee does not carry across invocations. When PackMds runs, it triggers BuildMds again, which overwrites the ESRP-signed DLLs with freshly-compiled unsigned ones. The unsigned DLLs then get packaged.
The -p:PackBuild=false property is passed here but is never checked anywhere in build2.proj — it has no effect.
Fix options:
- Add a condition to
build2.projso thatPackMdsskips itsDependsOnTargetswhenPackBuild=false:Or more simply, make<Target Name="PackMdsOnly" Condition="'$(PackBuild)' == 'false'"> ... </Target> <Target Name="PackMds" DependsOnTargets="BuildMds" Condition="'$(PackBuild)' != 'false'"> ... </Target>
DependsOnTargetsconditional. - Create a separate
PackMdsNoBuildtarget that only contains the packaging logic.
| - template: /eng/pipelines/onebranch/steps/publish-symbols-step.yml@self | ||
| parameters: | ||
| artifactName: 'Microsoft.Data.SqlClient_symbols_$(System.TeamProject)_$(Build.Repository.Name)_$(Build.SourceBranchName)_${{ parameters.mdsPackageVersion }}_$(System.TimelineId)' | ||
| azureSubscription: '${{ parameters.symbolsAzureSubscription }}' |
There was a problem hiding this comment.
Major: $(PACKAGE_NAME) is undefined — symbols will publish with an empty product name
The packageName parameter receives $(PACKAGE_NAME), but this variable is not defined in the job's variables block, in common-variables.yml, or in onebranch-variables.yml. It will resolve to an empty string at runtime, causing SymbolsProduct to be blank in the PublishSymbols@2 task.
Fix: Replace $(PACKAGE_NAME) with a literal value:
packageName: 'Microsoft.Data.SqlClient'
Not sure how the PackBuild target got lost, but I brought it back
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
eng/pipelines/onebranch/steps/esrp-nuget-signing-step.yml:36
searchPathis a new required parameter with no default. Existing callers that haven’t been updated (e.g., the csproj package job) will fail template expansion. Consider defaultingsearchPathto$(PACK_OUTPUT)to keep the template backward-compatible and reduce call-site boilerplate.
| variables: | ||
| ob_outputDirectory: '$(PACK_OUTPUT)' | ||
| ob_sdl_apiscan_softwareFolder: ${{ parameters.apiScanDllPath }} | ||
| ob_sdl_apiscan_symbolsFolder: ${{ parameters.apiScanPdbPath }} | ||
| ob_sdl_apiscan_softwarename: 'Microsoft.Data.SqlClient' | ||
| ob_sdl_apiscan_versionNumber: ${{ parameters.mdsAssemblyFileVersion }} | ||
|
|
There was a problem hiding this comment.
This job sets ob_sdl_apiscan_* folder/name/version variables but does not set ob_sdl_apiscan_enabled=true (which other signed-package jobs use). If OneBranch requires the per-job enable flag, APIScan may not run for the main SqlClient build. Add the enable variable here for consistency/compliance with the other build jobs.
| targetPath: '$(REPO_ROOT)/packages' | ||
|
|
||
| - task: DownloadPipelineArtifact@2 | ||
| displayName: Download Microsoft.Data.SqlClient.Extensions.Logging Artifact |
There was a problem hiding this comment.
DisplayName says "Microsoft.Data.SqlClient.Extensions.Logging" but the artifact being downloaded is the Internal.Logging package (per other pipeline naming). This is misleading in logs; rename the displayName to match the actual package/artifact.
| displayName: Download Microsoft.Data.SqlClient.Extensions.Logging Artifact | |
| displayName: Download Microsoft.Data.SqlClient.Internal.Logging Artifact |
| -p:PackageVersionLogging="${{ parameters.loggingPackageVersion }}" | ||
| -p:PackageVersionMds="${{ parameters.mdsPackageVersion }}" | ||
| -p:ReferenceType=Package | ||
| -p:SigningKeyPath="$(keyFile.secureFilePath)" |
There was a problem hiding this comment.
The Build2.proj invocation doesn’t pass a BuildNumber (or any assembly/file version override). build2.proj/MdsVersions.props default BuildNumber to 0, so the produced DLL FileVersion will likely be 7.0.0.0 and fail the signed-package validation that expects
| -p:SigningKeyPath="$(keyFile.secureFilePath)" | |
| -p:SigningKeyPath="$(keyFile.secureFilePath)" | |
| -p:BuildNumber=$(Build.BuildNumber) |
| msBuildCommandLine: >- | ||
| msbuild | ||
| $(REPO_ROOT)/build2.proj | ||
| -t:BuildMds | ||
| -p:Configuration=Release | ||
| -p:PackageVersionAbstractions="${{ parameters.abstractionsPackageVersion }}" | ||
| -p:PackageVersionLogging="${{ parameters.loggingPackageVersion }}" | ||
| -p:PackageVersionMds="${{ parameters.mdsPackageVersion }}" | ||
| -p:ReferenceType=Package |
There was a problem hiding this comment.
This Roslyn analyzer build of build2.proj also omits the BuildNumber (or any assembly/file version override). That means the analysis build will produce binaries with default version stamping (often BuildNumber=0), which can differ from the actual official build and make analyzer results harder to correlate. Consider passing the same BuildNumber property here as in the real build for consistency.
Description
This PR is the last functional PR for common project completion. It updates the official/non-official builds to use the targets from build2.proj, and effectively build the common project for official releases. I had plans for a larger overhaul of the "new" official pipeline, but I lost control of the direction, so I took a step back, pulled out the minimal changes to make this happen and am submitting this now.
Since the
*csproj*steps/jobs are based on build.proj, I've created separate steps for MDS build.🤖
Dropped "compound-" from steps files and fixed links. I thought I was onto something when I adopted that naming convention a while back, but ... no.
Issues
N/A
Testing
Currently running test build can he found:
https://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=144204&view=resultshttps://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=144572&view=resultshttps://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=144579&view=resultshttps://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=144586&view=results