Conversation
|
A Big YES to the last item in your checklist 🎉 |
paulmedynski
left a comment
There was a problem hiding this comment.
Loving the improvements and consolidation here - looking forward to porting this back to the release branches for a consistent experience.
A few questions and some comment typos to fix.
| -p:MdsPackageVersion=${{ parameters.mdsPackageVersion }} | ||
| -p:AkvPackageVersion=${{ parameters.akvPackageVersion }} | ||
| -p:PackageOutputPath=$(packagePath) | ||
| command: pack |
There was a problem hiding this comment.
I thought build2.proj had Pack targets...
There was a problem hiding this comment.
It does, but the pattern for the CI/PR pipelines seems to overwhelmingly be to directly build the csproj files. So rather than change the outlier (AKV Provider using build.proj) to a different outlier (AKV Provider using build2.proj), I changed it to follow the pattern for the other CI/PR pack tasks.
Please note that all of this will be obliterated as part of this semester's pipeline work :)
| configuration: '${{ parameters.buildConfiguration }}' | ||
| msbuildArguments: >- | ||
| -t:BuildAkvProvider | ||
| command: build |
There was a problem hiding this comment.
No Build target for AKV?
There was a problem hiding this comment.
Same comment as pack comment applies. Rather than move it from build.proj to build2.proj, I updated it to use the csproj directly like all the other projects in the CI/PR pipelines.
|
|
||
| pool: | ||
| name: Azure Pipelines | ||
| vmImage: ubuntu-latest |
There was a problem hiding this comment.
I believe we're supposed to use only our 1ES images. The other extensions packages are doing this wrong as well. Just FYI - we will fix this in a future PR once we have appropriate 1ES images setup.
There was a problem hiding this comment.
Well ... it's been like that since at least 2/24/26. 🤷♂️
Not sure why the entire file looks new - this was one of the commits I let the 🤖 handle.
| - name: packageName | ||
| # List of packages that the package being built with this job depends on. Each entry in the | ||
| # object should have the fields: | ||
| # * artifactName - Name of the artifact to published in a previous job that contains the desired |
There was a problem hiding this comment.
Name of the artifact pubished in a previous ...
| # Inform OneBranch that files put in this directory should be uploaded as artifacts. | ||
| ob_outputDirectory: $(PACK_OUTPUT) | ||
| # Placeholder for dependency package arguments. This will be constructed later as it depends | ||
| # on scripting to decompose parameters.dependencies. |
There was a problem hiding this comment.
parameters.dependencies.
There was a problem hiding this comment.
🤔 Isn't that what I have?
| publishSymbols: ${{ parameters.publishSymbols }} | ||
| downloadArtifacts: | ||
| dependencies: | ||
| - artifactName: '${{ parameters.loggingArtifactsName }}' |
There was a problem hiding this comment.
I like this approach - obvious and readable.
| ## Versioning | ||
|
|
||
| Versioning can be accomplished by using a mix of different parameters to the `build2.proj` targets: | ||
| `PackageVersion<TargetProject>`, `BuildNumber`, `and `BuildSuffix`. Using these in different combinations, can generate |
| ### Running Tests with Reference Type | ||
| The above documentation is the default mode of operation, and is the recommended mode for most developers. However, | ||
| `build2.proj` supports "package mode" builds. In this mode, instead of projects depending on other projects, they | ||
| depend on NuGet packages. This mode is useful for verifying that packages work witheach other, especially in automated |
| <Link>xunit.runner.json</Link> | ||
| </None> | ||
|
|
||
| <None Include="$(RepoRoot)src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/CodeCoverage.runsettings"> |
There was a problem hiding this comment.
Where did these copy commands go?
There was a problem hiding this comment.
Not necessary anymore, right? They got put into the build2.proj as what gets included if code coverage is turned on?
<TestCodeCoverage Condition="'$(TestCodeCoverage)' == ''">true</TestCodeCoverage>
<TestCodeCoverageArgument Condition="'$(TestCodeCoverage.ToLower())' == 'true'">
--collect "Code coverage"
--settings "$(RepoRoot)src/Microsoft.Data.SqlClient/tests/tools/Microsoft.Data.SqlClient.TestUtilities/CodeCoverage.runsettings"
</TestCodeCoverageArgument>
mdaigle
left a comment
There was a problem hiding this comment.
All the changes make sense to me. Look forward to seeing some pipeline runs.
| The above documentation is the default mode of operation, and is the recommended mode for most developers. However, | ||
| `build2.proj` supports "package mode" builds. In this mode, instead of projects depending on other projects, they | ||
| depend on NuGet packages. This mode is useful for verifying that packages work witheach other, especially in automated | ||
| build scenarios. For completeness, and debugging of autoamted builds, this section documents behavior of "package mode". |
| @@ -0,0 +1,311 @@ | |||
| # Test Guide for Microsoft.Data.SqlClient | |||
There was a problem hiding this comment.
Can we link to this from BUILDGUIDE rather than duplicating the content?
There was a problem hiding this comment.
For the sections I rewrote, yes, we have links to the TESTGUIDE.
For more
information about test procedures, including config file setup, see TESTGUIDE.md.
Manual test prerequisites and configuration are covered in TESTGUIDE.md.
For the stuff after the sections I rewrote ... well no, I didn't because I guess I figured everything after the sections I rewrote were just about managing runtime switches (which imo, doesn't seem like a great place to put them, but whatever - it's outside the scope of this PR). Since I had the 🤖 write the TESTGUIDE, I'll just tell it to include the stuff about perf testing and databases and whatever.
9c8cd1c to
6be4e7a
Compare
|
|
||
| | `<build_target>` | Description | | ||
| |:----------------------------|:--------------------------------------------------------------------------------| | ||
| | `Build` | Builds all projects for all platforms | |
There was a problem hiding this comment.
Is this also the default target?
We should support "msbuild" on the repository (as we've had before) that would build all projects and create NuGet packages for validation with default configuration setup.
| dotnet test "src/Microsoft.Data.SqlClient/tests/FunctionalTests/Microsoft.Data.SqlClient.FunctionalTests.csproj" \ | ||
| -p:Configuration=Release \ | ||
| --filter "category!=failing&category!=flaky&category!=interactive" | ||
| msbuild build2.proj -t:TestSqlClientUnit |
There was a problem hiding this comment.
There shouldn't be a need to specify name of proj file, if this is the only project file on root folder.
There was a problem hiding this comment.
Rename this to build.proj?
6be4e7a to
2112bb8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 78 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
.github/prompts/release-notes.prompt.md:35
- The Version Source table still points at files that were removed/renamed in this PR (
tools/props/Versions.props,*Versions.propsfiles). This will cause the release-notes prompt to guide agents to non-existent paths and incorrect version properties. Update the table to reference the new per-packageVersions.propsfiles (e.g.,src/Microsoft.Data.SqlClient/Versions.props,src/Microsoft.Data.SqlClient.Extensions/.../Versions.props, etc.) and the correct property names.
| Package | Version Source | Dependency Source |
|---------|---------------|-------------------|
| `Microsoft.Data.SqlClient` | [tools/props/Versions.props](tools/props/Versions.props) (`MdsVersionDefault`) | [Directory.Packages.props](Directory.Packages.props) and the [project file](src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj) |
| `AzureKeyVaultProvider` | [tools/props/Versions.props](tools/props/Versions.props) (`AkvVersionDefault`) | [AKV project file](src/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider/src/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj) and [Directory.Packages.props](Directory.Packages.props) |
| `Microsoft.SqlServer.Server` | [tools/props/Versions.props](tools/props/Versions.props) (`SqlServerPackageVersion`) | [SqlServer project file](src/Microsoft.SqlServer.Server/Microsoft.SqlServer.Server.csproj) |
| `Extensions.Abstractions` | [AbstractionsVersions.props](src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/AbstractionsVersions.props) | [Abstractions.csproj](src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj) |
| `Extensions.Azure` | [AzureVersions.props](src/Microsoft.Data.SqlClient.Extensions/Azure/src/AzureVersions.props) | [Azure.csproj](src/Microsoft.Data.SqlClient.Extensions/Azure/src/Azure.csproj) |
| `Internal.Logging` | [LoggingVersions.props](src/Microsoft.Data.SqlClient.Internal/Logging/src/LoggingVersions.props) | [Logging.csproj](src/Microsoft.Data.SqlClient.Internal/Logging/src/Logging.csproj) |
|
|
||
| pool: | ||
| name: Azure Pipelines | ||
| vmImage: ubuntu-latest |
There was a problem hiding this comment.
This job runs dotnet pack for Microsoft.SqlServer.Server (targets net46;netstandard2.0) on ubuntu-latest. Building net46 TFMs on Linux typically fails without additional reference-assembly packages/config, so this is very likely to break CI. Consider moving this job to a Windows pool (e.g., windows-latest) or adjusting the project/pack step to avoid net46 on non-Windows agents.
| vmImage: ubuntu-latest | |
| vmImage: windows-latest |
| # MDS library NuGet package version | ||
| # NOTE: This differs from the other structures! MdsVersions.props will deconstruct a provided | ||
| # package version and build an assembly version from it. If the build number is included | ||
| # before the "-" the build number will be appended again, generating an invalid file version. | ||
| # @TODO: This is a band-aid to ensure that CI builds until we can centralize versioning. | ||
| - name: mdsPackageVersion | ||
| value: 7.0.0-ci$(AssemblyBuildNumber) | ||
|
|
||
| # SqlServer library NuGet package version | ||
| # NOTE: SqlServer's version props derive file version from the package version by appending the | ||
| # assembly build number, so this must remain a three-part SemVer base with a prerelease | ||
| # suffix, not a four-part numeric version. | ||
| - name: sqlServerPackageVersion | ||
| value: 1.0.0-ci$(AssemblyBuildNumber) |
There was a problem hiding this comment.
mdsPackageVersion/sqlServerPackageVersion are using $(AssemblyBuildNumber), but this file defines the build-number variable as assemblyBuildNumber (lowercase). Even if ADO macro expansion treats names case-insensitively, this is inconsistent with the rest of the file and easy to misread. Also, the comment above mdsPackageVersion still references MdsVersions.props, but the repo now uses Versions.props/SqlClientPackageVersion. Please align the variable name usage and update the comment to match the current versioning files/properties.
| <!-- Put the ThisAssembly class in the assembly's name namespace by default. --> | ||
| <ThisAssemblyNamespace Condition="'$(ThisAssemblyNamespace)' == ''">$(AssemblyName)</ThisAssemblyNamespace> |
There was a problem hiding this comment.
Changing the default ThisAssembly namespace from System to $(AssemblyName) is a breaking behavior change for any code that reflects or references System.ThisAssembly (e.g., the stress runner currently looks up assembly.GetType("System.ThisAssembly") and will now return null). Either keep System as the default for backward-compat (and explicitly override ThisAssemblyNamespace in the projects that need uniqueness) or update all downstream consumers that assume System.ThisAssembly.
2112bb8 to
3976008
Compare
| configurationToPack: ${{ parameters.buildConfiguration }} | ||
| packDirectory: $(dotnetPackagesDir) | ||
| verbosityToPack: ${{ parameters.dotnetVerbosity }} | ||
| buildProperties: SqlServerPackageVersion=${{ parameters.sqlServerPackageVersion }} |
There was a problem hiding this comment.
dotnet pack only passes SqlServerPackageVersion, but src/Microsoft.SqlServer.Server/Versions.props computes SqlServerFileVersion by appending $(FileVersionBuildNumber) (derived from $(BuildNumber)). Since BuildNumber isn't provided here, FileVersionBuildNumber falls back to 0 and file versions will come out as x.y.z.0. Pass BuildNumber=$(Build.BuildNumber) (and any other required version props) via buildProperties so file versions reflect the build.
| buildProperties: SqlServerPackageVersion=${{ parameters.sqlServerPackageVersion }} | |
| buildProperties: SqlServerPackageVersion=${{ parameters.sqlServerPackageVersion }};BuildNumber=$(Build.BuildNumber) |
| | Package | Version Source | Dependency Source | | ||
| |---------|---------------|-------------------| | ||
| | `Microsoft.Data.SqlClient` | [tools/props/Versions.props](tools/props/Versions.props) (`MdsVersionDefault`) | [Directory.Packages.props](Directory.Packages.props) and the [project file](src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj) | | ||
| | `AzureKeyVaultProvider` | [tools/props/Versions.props](tools/props/Versions.props) (`AkvVersionDefault`) | [AKV project file](src/Microsoft.Data.SqlClient/add-ons/AzureKeyVaultProvider/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj) and [Directory.Packages.props](Directory.Packages.props) | | ||
| | `AzureKeyVaultProvider` | [tools/props/Versions.props](tools/props/Versions.props) (`AkvVersionDefault`) | [AKV project file](src/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider/src/Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider.csproj) and [Directory.Packages.props](Directory.Packages.props) | | ||
| | `Microsoft.SqlServer.Server` | [tools/props/Versions.props](tools/props/Versions.props) (`SqlServerPackageVersion`) | [SqlServer project file](src/Microsoft.SqlServer.Server/Microsoft.SqlServer.Server.csproj) | | ||
| | `Extensions.Abstractions` | [AbstractionsVersions.props](src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/AbstractionsVersions.props) | [Abstractions.csproj](src/Microsoft.Data.SqlClient.Extensions/Abstractions/src/Abstractions.csproj) | | ||
| | `Extensions.Azure` | [AzureVersions.props](src/Microsoft.Data.SqlClient.Extensions/Azure/src/AzureVersions.props) | [Azure.csproj](src/Microsoft.Data.SqlClient.Extensions/Azure/src/Azure.csproj) | |
There was a problem hiding this comment.
The versioning/dependency source table is now stale: it still points at tools/props/Versions.props and *Versions.props files that were removed/renamed in this PR (e.g., SqlClient now uses src/Microsoft.Data.SqlClient/Versions.props, and Abstractions/Azure version files are now src/.../Versions.props). Update the table to reference the current versioning sources so the prompt doesn’t direct agents to non-existent files.
| <PackageVersion | ||
| Include="Microsoft.Data.SqlClient" | ||
| Version="$(MdsPackageVersion)" /> | ||
| Version="$(SqlClientPackageVersion)" /> | ||
| <PackageVersion | ||
| Include="Microsoft.Data.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider" | ||
| Version="$(AkvPackageVersion)" /> | ||
| Version="$(AkvProviderPackageVersion)" /> |
There was a problem hiding this comment.
In Package reference builds, Microsoft.Data.SqlClient now takes a package reference on Microsoft.SqlServer.Server (see src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj), and CI now builds/publishes a SqlServer artifact with version $(sqlServerPackageVersion) (e.g., 1.0.0-ci...). However, Directory.Packages.props hardcodes Microsoft.SqlServer.Server to 1.0.0, which will cause restore to look for the wrong version when consuming the locally-built artifact. Consider versioning Microsoft.SqlServer.Server here via a $(SqlServerPackageVersion) property (and passing it in CI) for consistency with the other in-repo packages.
…r to build2.proj norms
…csproj, just like all the other projects.
Migrate the SqlClient csproj files to the same pattern as the other projects.
…ctory.Build.props
dea9239 to
1f5e944
Compare
Description
This PR moves all projects in the repository to be built, tested, and packed with build2.proj. This style was pioneered with the merge project and this PR migrates all the other projects to this format. Versioning has been updated for each package as well, and explained thoroughly in BUILDGUIDE.md. A new TESTGUIDE.md has been added as well.
🤖
Testing
Local tests of build2.proj is working as intended
Non-official build pipeline is passing:
https://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=148136&view=resultshttps://sqlclientdrivers.visualstudio.com/ADO.Net/_build/results?buildId=148854&view=results
CI/PR pipeline validation is forthcoming.
Work to do before completing PR
[ ] Validate PR/CI pipeline
[ ] Validate Non-official pipeline
[ ] Update agent instructions
[x] Rename build2.proj to build.proj?