Skip to content

release/dotnetup uses dotnetup to Install the SDK + Daily Version Parsing Fix#54469

Merged
nagilson merged 12 commits into
dotnet:release/dnupfrom
nagilson:nagilson-use-dotnetup-to-install-sdk
May 28, 2026
Merged

release/dotnetup uses dotnetup to Install the SDK + Daily Version Parsing Fix#54469
nagilson merged 12 commits into
dotnet:release/dnupfrom
nagilson:nagilson-use-dotnetup-to-install-sdk

Conversation

@nagilson
Copy link
Copy Markdown
Member

@nagilson nagilson commented May 27, 2026

The first part of #54257

  1. Modifies the setup scripts to install the SDK before arcade tooling gets a chance to do so, causing the install script invocation to be a no-op.

  2. This fixes a bug which would cause the version parsing to not download the correct version. This PR needs to be merged for the script to actually work because the downloaded version of dotnetup at this time will not contain the bug fix.

  3. I found --no-progress is a bogus flag now and needs to be fixed so I removed it from the CI call: dotnetup --no-progress shows progress (and poorly formatted progress at that) #54468

potentially contentious:

  1. I made it not reinstall dotnetup if its less than 24hr old by default, but made this overridable with an environment var that persists in CI at this time.

@nagilson nagilson marked this pull request as ready for review May 27, 2026 17:45
Copilot AI review requested due to automatic review settings May 27, 2026 17:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the release/dotnetup toolset bootstrap flow to use dotnetup for installing the SDK early (before Arcade’s toolset initialization), and fixes channel parsing so fully-qualified prerelease SDK versions (with dotted prerelease segments) resolve correctly.

Changes:

  • Install the global.json-pinned bootstrap SDK into the repo-local .dotnet via dotnetup during toolset configuration (with VMR/source-build and non-restore guards).
  • Fix ChannelVersionResolver parsing to properly recognize fully-qualified prerelease SDK versions (e.g., 10.0.100-preview.1.32640) as “fully specified”.
  • Improve get-dotnetup scripts by adding a “freshness” short-circuit (24h) with --force/-Force + DOTNETUP_FORCE_REINSTALL=1 override, and remove --no-progress from CI toolset installs.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/dotnetup.Tests/ChannelVersionResolverTests.cs Adds coverage ensuring fully-qualified prerelease channel strings resolve to the exact version.
src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs Fixes parsing so prerelease suffix dots don’t break version-part splitting; treats prerelease fully-qualified versions as fully specified.
scripts/get-dotnetup.sh Adds “skip download if recent binary exists” behavior and a --force override.
scripts/get-dotnetup.ps1 Adds “skip download if recent binary exists” behavior with -Force override; computes SHA-512 via .NET instead of Get-FileHash.
eng/restore-toolset.sh Removes --no-progress from dotnetup runtime install invocation.
eng/restore-toolset.ps1 Removes --no-progress from dotnetup runtime install invocation and seeds $LASTEXITCODE to avoid strict-mode issues when scripts short-circuit.
eng/configure-toolset.sh Adds dotnetup-based bootstrap SDK installation into repo-local .dotnet during restore (with CI forcing re-download).
eng/configure-toolset.ps1 Adds dotnetup-based bootstrap SDK installation into repo-local .dotnet during restore (with CI forcing re-download) and telemetry/exit-code handling.

@nagilson nagilson requested a review from a team May 27, 2026 18:06
Comment thread eng/configure-toolset.ps1 Outdated
Comment thread eng/configure-toolset.sh Outdated
@dsplaisted
Copy link
Copy Markdown
Member

This fixes a bug which would cause the version parsing to not download the correct version. This PR needs to be merged for the script to actually work because the downloaded version of dotnetup at this time will not contain the bug fix.

So wouldn't the version parsing fix need to be merged separately from the script changes for things to work? Otherwise the script would be merged but not working.

@nagilson
Copy link
Copy Markdown
Member Author

So wouldn't the version parsing fix need to be merged separately from the script changes for things to work? Otherwise the script would be merged but not working.

  1. I tested the script by building and replacing the downloaded copy with my local build
  2. The sdk itself will still get downloaded if the script does not work by the arcade tool
  3. No, the new dotnetup version will be downloaded from the aka.ms link with the fix, making the script work at the same time it is merged, unless im mistaken

@dsplaisted
Copy link
Copy Markdown
Member

I made it not reinstall dotnetup if its less than 24hr old by default, but made this overridable with an environment var that persists in CI at this time.

I don't think we should change the default for the get-dotnetup scripts. It's fine for them to allow an option to control this, and the CI restore-toolset can set that option by default, but I think the default if you download and run the scripts should be that you always get the latest version of dotnetup.

@nagilson
Copy link
Copy Markdown
Member Author

Good call, I will move that logic to be part of our tooling but not part of the script. I agree it should not be part of the script.

- get-dotnetup.ps1/sh now always download dotnetup (no freshness check).
  CI caching policy belongs in engineering infra, not the install script.
- Remove DOTNETUP_FORCE_REINSTALL env var logic from configure-toolset.
- Restructure configure-toolset to collect all SDK versions (primary +
  additionalDotNetVersions from global.json) and install them in a
  single dotnetup invocation for better perf.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dsplaisted
Copy link
Copy Markdown
Member

So wouldn't the version parsing fix need to be merged separately from the script changes for things to work? Otherwise the script would be merged but not working.

  1. I tested the script by building and replacing the downloaded copy with my local build
  2. The sdk itself will still get downloaded if the script does not work by the arcade tool
  3. No, the new dotnetup version will be downloaded from the aka.ms link with the fix, making the script work at the same time it is merged, unless im mistaken

I think this PR should be failing because it should be getting the unfixed version of dotnetup and so dotnetup is failing to install the SDK. It sounds like then it's falling back to the arcade logic to install the SDK, but I think it should fail the build and abort instead. We want to use this for validation that dotnetup works, but if it's going to fail and silently fall back to the dotnet-install scripts, then we're not really getting that validation.

@nagilson
Copy link
Copy Markdown
Member Author

nagilson commented May 28, 2026

@dsplaisted

We want to use this for validation that dotnetup works, but if it's going to fail and silently fall back to the dotnet-install scripts, then we're not really getting that validation.

I totally get where you're coming from.

From my perspective we still get the validation - telemetry is visible to us (at least while the data is not being corrupted by a new data transformation service but we shouldn't expect that to happen often) and we have data on the % success of each command, as well as the error breakdowns for each command.

For example, I see that this error was hit 3 times in CI and is a top contender for sdk install failures:

InvalidOperation

  at Microsoft.DotNet.Tools.Bootstrapper.DotnetupPaths.get_DataDirectory()
   at Microsoft.DotNet.Tools.Bootstrapper.InstallerOrchestratorSingleton.PrepareInstall(ResolvedInstallRequest, InstallBatchContext, InstallResult&)
   at Microsoft.DotNet.Tools.Bootstrapper.InstallerOrchestratorSingleton.<>c__DisplayClass8_0.<PrepareConcurrent>b__0(ResolvedInstallRequest)

As soon as signing is merged, I'd like to scope this out to the SDK repo itself. There, we will be able to get this data without causing pain for people (arcade fallback will still work) if we have a bug. The same is true for when we roll out to Arcade. Once we have high adoption and high numbers, then I'd want to remove the arcade fallback.

What do you think?

I can see on one side that having it fail the build would make it more obvious and urgent to fix or potentially easier to spot right there in the failing build, but the failure is still visible with the arcade fallback locally. This also makes it simpler because I can apply the same patch in the release/dnup branch as sdk main and then arcade

@dsplaisted
Copy link
Copy Markdown
Member

I am OK either way but would still choose failing fast myself, especially in the release/dnup branch. As we move to main and other repos we might want to be more forgiving, but hopefully dotnetup is at least as reliable as the install scripts so it should be moot.

nagilson added 2 commits May 28, 2026 11:17
dotnetup already checks if the sdk exists, no need to dupe that logic

and the 24hr check was supposed to be present, just not within the dotnetup scripts themselves - this is great feedback. I think I will keep it locally because it makes the build of dotnetup and the .net sdk when working locally as a dev take way longer than necessary when dotnetup is not  changing that often
@nagilson
Copy link
Copy Markdown
Member Author

I am OK either way but would still choose failing fast myself, especially in the release/dnup branch. As we move to main and other repos we might want to be more forgiving, but hopefully dotnetup is at least as reliable as the install scripts so it should be moot.

That's totally fair, I don't think either of us feel super strongly about it.

Thanks for leaving that feedback; I'll leave it as is but I can go either way too. It's easier to keep all of the scripts the same rather than have that nuance of modifying the behavior when we port it in and out.

@nagilson nagilson enabled auto-merge May 28, 2026 18:26
@nagilson nagilson disabled auto-merge May 28, 2026 21:26
@nagilson nagilson merged commit 74a5ae6 into dotnet:release/dnup May 28, 2026
20 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants