Fix SDK version check failing after private/custom dotnet SDK installation#18213
Open
CloudColonel wants to merge 5 commits into
Open
Fix SDK version check failing after private/custom dotnet SDK installation#18213CloudColonel wants to merge 5 commits into
CloudColonel wants to merge 5 commits into
Conversation
- Add IDotNetRuntimeSelector and DotNetRuntimeSelector for SDK mode management - Add IProcessLauncher and ProcessLauncher for process execution with correct runtime - Add AspireSettings schema for settings.json configuration support - Update SdkInstallHelper to support installation via runtime selector - Register new services in Program.cs DI container - Add comprehensive tests for new components - Support environment variables for configuration overrides - Implement Spectre.Console UX for interactive installation prompts Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
- Modify DotNetCliRunner to use dotnet executable path from runtime selector - Add environment variables from runtime selector to process execution - Add SdkLockHelper for concurrent SDK installation protection - Update DotNetRuntimeSelector to use locking during installation - Add double-check pattern to prevent duplicate installations - Maintain backward compatibility with existing process launching logic Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
…ation After DotNetRuntimeSelector installs or locates a private/custom SDK, subsequent calls to DotNetSdkInstaller.CheckAsync were still invoking the system "dotnet" binary instead of the newly configured one, causing a spurious minimum-SDK-version error (e.g. on `aspire new`). - Added SetDotNetExecutablePath() to DotNetSdkInstaller so the executable used for SDK checks can be updated at runtime. - DotNetRuntimeSelector now calls SetDotNetExecutablePath() (via a safe DotNetSdkInstaller cast) whenever it switches to a private or custom SDK path. - Removed the redundant second sdkInstaller.CheckAsync() call in SdkInstallHelper.EnsureSdkInstalledAsync; InitializeAsync() already verifies the SDK is available, so the extra check was unnecessary and used the wrong path. Co-Authored-By: Claude <noreply@anthropic.com>
CloudColonel
requested review from
JamesNK,
davidfowl and
mitchdenny
as code owners
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18213Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18213" |
|
@CloudColonel please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces .NET runtime selection capabilities to the Aspire CLI, allowing the tool to use either a system-installed .NET SDK, a private SDK installed under ~/.aspire/sdk, or a custom user-specified path, with automatic installation support for private SDKs.
Changes:
- Adds
IDotNetRuntimeSelector/DotNetRuntimeSelectorfor detecting, selecting, and optionally installing a private .NET SDK - Introduces
IProcessLauncher/ProcessLauncherandSdkLockHelperutilities for launching processes with the correct runtime and coordinating concurrent SDK installations - Updates
DotNetCliRunnerandDotNetSdkInstallerto use the selected runtime path and environment variables
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/DotNet/IDotNetRuntimeSelector.cs | New interface for runtime selection |
| src/Aspire.Cli/DotNet/DotNetRuntimeSelector.cs | Implementation with system/private/custom SDK logic |
| src/Aspire.Cli/DotNet/DotNetSdkInstaller.cs | Made dotnet path configurable via SetDotNetExecutablePath |
| src/Aspire.Cli/DotNet/DotNetCliRunner.cs | Uses runtime selector for dotnet path and env vars |
| src/Aspire.Cli/Utils/IProcessLauncher.cs | New interface for process launching |
| src/Aspire.Cli/Utils/ProcessLauncher.cs | Implementation applying runtime env vars |
| src/Aspire.Cli/Utils/SdkLockHelper.cs | File-based lock to prevent concurrent SDK installs |
| src/Aspire.Cli/Utils/SdkInstallHelper.cs | New overload delegating SDK check to runtime selector |
| src/Aspire.Cli/Configuration/AspireSettings.cs | Settings model for ~/.aspire/settings.json |
| src/Aspire.Cli/JsonSourceGenerationContext.cs | Registers AspireSettings for JSON source gen |
| src/Aspire.Cli/Program.cs | DI registration and startup initialization |
| tests/Aspire.Cli.Tests/Utils/ProcessLauncherTests.cs | Tests for process launcher |
| tests/Aspire.Cli.Tests/DotNet/DotNetRuntimeSelectorTests.cs | Tests for runtime selector |
Comment on lines
+32
to
+60
| while (File.Exists(lockFilePath)) | ||
| { | ||
| if (DateTime.UtcNow - startTime > maxWaitTime) | ||
| { | ||
| // Remove stale lock file if it's too old (more than 30 minutes) | ||
| var lockFileInfo = new FileInfo(lockFilePath); | ||
| if (DateTime.UtcNow - lockFileInfo.CreationTimeUtc > TimeSpan.FromMinutes(30)) | ||
| { | ||
| try | ||
| { | ||
| File.Delete(lockFilePath); | ||
| break; | ||
| } | ||
| catch | ||
| { | ||
| // If we can't delete it, another process might be using it | ||
| } | ||
| } | ||
|
|
||
| throw new TimeoutException($"Timeout waiting for SDK installation lock for version {sdkVersion}"); | ||
| } | ||
|
|
||
| await Task.Delay(1000, cancellationToken); | ||
| } | ||
|
|
||
| // Create lock file | ||
| await File.WriteAllTextAsync(lockFilePath, | ||
| $"Locked by process {Environment.ProcessId} at {DateTime.UtcNow:O}", | ||
| cancellationToken); |
Comment on lines
+340
to
+349
| using var process = new Process { StartInfo = startInfo }; | ||
| process.Start(); | ||
|
|
||
| await process.WaitForExitAsync(cancellationToken); | ||
|
|
||
| if (process.ExitCode != 0) | ||
| { | ||
| var error = await process.StandardError.ReadToEndAsync(cancellationToken); | ||
| throw new InvalidOperationException($"dotnet-install script failed with exit code {process.ExitCode}: {error}"); | ||
| } |
Comment on lines
+216
to
+218
| // Initialize the .NET runtime selector | ||
| var runtimeSelector = app.Services.GetRequiredService<IDotNetRuntimeSelector>(); | ||
| await runtimeSelector.InitializeAsync(); |
Comment on lines
+88
to
+114
| public override async Task<int> LaunchAsync( | ||
| string executablePath, | ||
| string? arguments = null, | ||
| string? workingDirectory = null, | ||
| IDictionary<string, string>? environmentVariables = null, | ||
| CancellationToken cancellationToken = default) | ||
| { | ||
| LastExecutablePath = executablePath; | ||
| LastArguments = arguments; | ||
| LastWorkingDirectory = workingDirectory; | ||
|
|
||
| // Combine runtime env vars and additional ones just like the base class | ||
| var runtimeEnvVars = _runtimeSelector.GetEnvironmentVariables(); | ||
| var combined = new Dictionary<string, string>(runtimeEnvVars); | ||
|
|
||
| if (environmentVariables != null) | ||
| { | ||
| foreach (var kvp in environmentVariables) | ||
| { | ||
| combined[kvp.Key] = kvp.Value; | ||
| } | ||
| } | ||
|
|
||
| LastEnvironmentVariables = combined; | ||
|
|
||
| return await Task.FromResult(0); | ||
| } |
Comment on lines
+230
to
+246
| if (File.Exists(privateDotNetPath)) | ||
| { | ||
| _mode = DotNetRuntimeMode.Private; | ||
| _dotNetExecutablePath = privateDotNetPath; | ||
|
|
||
| // Set environment variables to isolate the private SDK | ||
| _environmentVariables["DOTNET_ROOT"] = privateSdkPath; | ||
| _environmentVariables["DOTNET_HOST_PATH"] = privateDotNetPath; | ||
|
|
||
| // Update the SDK installer so subsequent CheckAsync calls use the private dotnet executable. | ||
| if (sdkInstaller is DotNetSdkInstaller concreteInstaller) | ||
| { | ||
| concreteInstaller.SetDotNetExecutablePath(privateDotNetPath); | ||
| } | ||
|
|
||
| return true; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
aspire newshowed an error that the minimum .NET SDK version is not installed, even though the correct SDK was just installed.Fix
DotNetSdkInstallernow uses the dotnet executable path configured byDotNetRuntimeSelector(private or custom SDK path), rather than always using the systemdotnet.SetDotNetExecutablePath()toDotNetSdkInstallerso the executable used for SDK checks can be updated at runtime.DotNetRuntimeSelectornow callsSetDotNetExecutablePath()(via a safeDotNetSdkInstallercast) whenever it switches to a private or custom SDK path.sdkInstaller.CheckAsync()call inSdkInstallHelper.EnsureSdkInstalledAsync;InitializeAsync()already verifies the SDK is available, so the extra check was unnecessary and used the wrong path.🤖 Generated with Claude Code