don't let dotnetup failure count as a install script failure - #54756
Conversation
… not just exit code but folder existence on disk
Empirical Verification ✅Tested the PR fix by forcing dotnetup to fail during SDK build (replaced the dotnetup binary path with a fake .cmd that exits with code 1). Results1. dotnetup failure was detected and reported: 2. Fallback to dotnet-install script succeeded — build was NOT cancelled: 3. MSBuild compilation output exists — build continued past the install phase: The build only failed due to an unrelated pre-existing error ( ConclusionThe fix works correctly: when dotnetup fails, the install script fallback runs, succeeds, and the build continues to MSBuild compilation without being cancelled. |
…x-main-restore-toolset-exitcode
Re-verification ✅ (latest script)Re-ran the empirical test on the latest commit (95a31ea). Forced dotnetup to fail via a fake �xit /b 1 binary and removed the cached 6.0 runtime to trigger the install path. 1. dotnetup failure detected → fallback triggered: 2. Fallback install script succeeded: 3. MSBuild compilation output exists — build continued: Build only failed due to pre-existing unrelated error ( |
95a31ea to
51c8a32
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts the toolset restore scripts to ensure that a failing dotnetup attempt doesn’t incorrectly cause the subsequent dotnet-install fallback path to be treated as failed, and adds filesystem-based validation that a runtime was actually installed.
Changes:
- Add shared-framework path/name helpers and reuse them when deciding whether runtimes need installation.
- In the dotnet-install fallback path, reset/capture exit codes locally and validate installation by checking the expected shared framework folder exists.
- Improve failure messaging to include more context (arch + “installed” boolean).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| eng/restore-toolset.sh | Adds shared-framework helpers, improves runtime presence checks, and makes install-script error handling + validation more robust. |
| eng/restore-toolset.ps1 | Mirrors the shell changes in PowerShell: shared-framework helpers, proper exit-code handling, and post-install validation. |
Re-verification on latest code ✅ (commit 51c8a32)Pulled the new commit ( 1. dotnetup failure detected → fallback triggered: 2. Fallback install script succeeded: 3. MSBuild compilation output exists — build continued past install phase: Build only failed due to pre-existing unrelated error ( |
dsplaisted
left a comment
There was a problem hiding this comment.
If I understand correctly, the failure to install with dotnetup was propagating through even though the subsequent dotnet-install script succeeded. It looks like this fixes that error propagation, and adds additional sanity validation that the shared framework was installed.
Do we need that extra validation? Do we not trust the successful return code from the install script?
The issue is that in powershell $LASTEXITCODE is only set by native/external executables ( .exe , cmd , etc.). It is never modified by: • PowerShell cmdlets ( Invoke-WebRequest , Write-Host , Remove-Item ) The install script, thus, does not set this, which makes us think that the installation failed if dotnetup failed. |
Exhibited CI issue in #54752.
The
exitcodeset bydotnetupoverrides whether thecithinks the install script succeeded or not, when it did succeed here, but the CI thought it failed. We also now validate the folder rather than assumingexit code 0 = a happy path(I sound like claude in that sentence which is sad.)