[DRAFT] Use global.json to restrict .NET SDK use #3797
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces global.json to standardize the .NET SDK version (9.0.0) across the project and migrates pipeline files to use UseDotNet@2 with useGlobalJson: true instead of hardcoded version numbers or the ensure-dotnet-version.yml template. The changes simplify SDK installation, remove unnecessary runtime installations in build-only scenarios, and reduce redundant .NET Framework test targets in the stress tests.
Key changes:
- Added
global.jsonspecifying .NET 9.0 SDK withrollForward: "feature"policy - Updated 6 pipeline files to use
UseDotNet@2withuseGlobalJson: true - Removed installation of .NET 8.x runtime in build-only scenarios where tests aren't run
- Reduced stress test .NET Framework targets from 6 versions to just net462
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| global.json | New file specifying .NET 9.0.0 SDK requirement with feature rollforward and no prerelease versions |
| eng/pipelines/steps/compound-build-akv-step.yml | Migrated from hardcoded SDK/runtime installation to global.json; removed unnecessary .NET 8.x runtime |
| eng/pipelines/stages/stress-tests-ci-stage.yml | Reduced .NET Framework test targets from 6 versions to just net462 |
| eng/pipelines/jobs/stress-tests-ci-job.yml | Updated to use global.json for SDK installation and changed .NET 8 runtime from '8.x' to '8.0' |
| eng/pipelines/dotnet-sqlclient-ci-core.yml | Added net9.0 to x86 test target frameworks with explanatory comments |
| eng/pipelines/common/templates/steps/build-all-configurations-signed-dlls-step.yml | Migrated to global.json; removed unnecessary .NET 8.x runtime installation |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Simplified x86 SDK/runtime installation using global.json and added .NET 8.0 runtime for tests |
| eng/pipelines/common/templates/jobs/ci-code-coverage-job.yml | Replaced ensure-dotnet-version template with UseDotNet@2 using global.json |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| parameters: | ||
| packageType: 'sdk' | ||
| version: '10.0' | ||
| # Install whichever .NET SDKs are specified in our global.json. |
There was a problem hiding this comment.
Code coverage only runs on Windows x64, so we don't need the special ensure-dotnet.version template here.
| packageType: sdk | ||
| version: '9.x' | ||
|
|
||
| - task: UseDotNet@2 |
There was a problem hiding this comment.
We don't need a .NET 8 runtime to build things. We would only need it to run tests, which doesn't happen as part of this pipeline tree.
| packageType: 'sdk' | ||
| version: '9.x' | ||
|
|
||
| - task: UseDotNet@2 |
There was a problem hiding this comment.
We don't need a .NET 8 runtime to build things. We would only need it to run tests, which doesn't happen as part of this pipeline tree.
global.json
Outdated
| "sdk": | ||
| { | ||
| // We currently require the .NET 9 SDK to build and test the project. | ||
| "version": "9.0.0", |
There was a problem hiding this comment.
@priyankatiwari08 - You will need to set this to 10.0.100 in your PR.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
eng/pipelines/jobs/stress-tests-ci-job.yml:146
- With the stress tests now targeting net9.0 (as specified in netTestRuntimes parameter which defaults to [net8.0, net9.0]), the .NET 9.0 runtime should also be installed. Consider adding:
# Install the .NET 9.0 runtime to run tests that target it.
- task: UseDotNet@2
displayName: Install .NET 9.0 Runtime
inputs:
packageType: runtime
version: 9.x # Install the .NET 8.0 runtime to run tests that target it.
- task: UseDotNet@2
displayName: Install .NET 8.0 Runtime
inputs:
packageType: runtime
version: 8.x
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 16 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Commentary for reviewers.
| - name: timeout | ||
| type: number | ||
|
|
||
| # True if this is an ARM64 job. |
There was a problem hiding this comment.
We don't need to detect ARM64 - we already know explicitly which agent images are ARM64 and we can simply set this flag in those cases.
| downloadedNugetPath: $(Pipeline.Workspace)\${{parameters.artifactName }} | ||
| debug: ${{ parameters.debug }} | ||
|
|
||
| - ${{ else }}: # project |
There was a problem hiding this comment.
It was weird for the pre-build step to run a build. This has been moved out to the calling template, along with the installation of the SDKs and Runtimes.
| - allNoDocs | ||
|
|
||
| steps: | ||
| - template: ./ensure-dotnet-version.yml@self |
There was a problem hiding this comment.
This now happens earlier in the template tree.
| images: # list of images to run tests on | ||
| Win22_Sql19_x86: ADO-MMS22-SQL19 # stage display name: image name from the pool | ||
| TargetFrameworks: [net8.0] #[net462, net8.0] # list of target frameworks to run | ||
| TargetFrameworks: [net462, net8.0, net9.0] |
There was a problem hiding this comment.
Why not test all of the runtimes we support?
| pool: ADO-CI-PUBLIC-ARM64-1ES-EUS-POOL | ||
| images: | ||
| Win22_Azure_ARM64_Sql: ADO-WIN11-ARM64 | ||
| architecture: arm64 |
There was a problem hiding this comment.
This is the only agent image that is ARM64, so we can simply tell the downstream templates explicitly ratherr than trying to detect agent OS and architecture.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
eng/pipelines/jobs/stress-tests-ci-job.yml:145
- The stress test job installs the .NET 8.0 runtime but not the .NET 9.0 runtime. Since the stress tests target net9.0 (as seen in stress-tests-ci-stage.yml line 57 which defaults to
[net8.0, net9.0]), the .NET 9.0 runtime should also be installed.
Add a step after line 144 to install the .NET 9.0 runtime:
- task: UseDotNet@2
displayName: Install .NET 9.0 Runtime
inputs:
packageType: runtime
version: 9.x - task: UseDotNet@2
displayName: Install .NET 8.0 Runtime
inputs:
packageType: runtime
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
…on, currently 9.0. - Using global.json to install .NET SDKs everywhere except when we may need ARM64. - Removed unnecessary installation of .NET SDKs and runtimes where they aren't needed. - Added all target frameworks to the x86 test runs. - Updated documentation related to SDKs. - Removed runtime ARM64 architecture detection in favour of explicit parameters for the test jobs that use ARM64. - Suppressing CredScan errors related to test secrets.
paulmedynski
force-pushed
the
dev/paul/global-json
branch
from
1388f65 to
bf917e9
Compare
Description
Testing
PR, CI, and Official pipelines will validate.