Fix post-private-SDK-install false negative by routing SDK availability check through IDotNetRuntimeSelector#18115
Open
CloudColonel wants to merge 5 commits into
Open
Fix post-private-SDK-install false negative by routing SDK availability check through IDotNetRuntimeSelector#18115CloudColonel 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>
…ty check through IDotNetRuntimeSelector 1. Commands (New, Run, Add, Exec, Publish, Deploy) now inject IDotNetRuntimeSelector and call EnsureSdkInstalledAsync with 3 args 2. SdkInstallHelper.EnsureSdkInstalledAsync (3-arg) now trusts runtimeSelector.InitializeAsync() result directly - no redundant system-PATH check via DotNetSdkInstaller after private SDK install 3. DotNetRuntimeSelector.InitializeAsync() is now idempotent (caches result on first call) 4. Program.Main() no longer calls runtimeSelector.InitializeAsync() redundantly - commands handle it 5. Tests: Added TestDotNetRuntimeSelector, updated SdkInstallerTests to configure both DotNetSdkInstaller and DotNetRuntimeSelector for "SDK not installed" scenarios Co-Authored-By: Claude <noreply@anthropic.com>
CloudColonel
requested review from
JamesNK,
davidfowl and
mitchdenny
as code owners
|
@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 a .NET Runtime Selector feature to the Aspire CLI, enabling the CLI to select between system-installed, privately-installed, or custom .NET SDK runtimes. When the system SDK is unavailable, it can offer to install a private SDK under ~/.aspire/sdk.
Changes:
- Adds
IDotNetRuntimeSelector/DotNetRuntimeSelectorfor selecting and managing .NET runtime mode (system, private, or custom), with settings-based and environment-variable-based configuration. - Adds
IProcessLauncher/ProcessLauncherandSdkLockHelperto support launching processes with the correct runtime and managing concurrent SDK installations. - Updates all CLI commands to use the new runtime selector through
SdkInstallHelper.EnsureSdkInstalledAsync.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/DotNet/IDotNetRuntimeSelector.cs | Interface for runtime selection with mode, path, init, and env vars |
| src/Aspire.Cli/DotNet/DotNetRuntimeSelector.cs | Implementation handling system/private/custom SDK resolution and installation |
| src/Aspire.Cli/Utils/IProcessLauncher.cs | Interface for launching processes with the correct runtime |
| src/Aspire.Cli/Utils/ProcessLauncher.cs | Implementation applying runtime env vars and executable path |
| src/Aspire.Cli/Utils/SdkLockHelper.cs | File-based lock to prevent concurrent private SDK installations |
| src/Aspire.Cli/Utils/SdkInstallHelper.cs | New overload of EnsureSdkInstalledAsync using runtime selector |
| src/Aspire.Cli/Configuration/AspireSettings.cs | Settings model for ~/.aspire/settings.json |
| src/Aspire.Cli/JsonSourceGenerationContext.cs | Registers AspireSettings for source-generated JSON |
| src/Aspire.Cli/Program.cs | Registers new services in DI container |
| src/Aspire.Cli/DotNet/DotNetCliRunner.cs | Uses runtime selector for dotnet executable path and env vars |
| src/Aspire.Cli/Commands/RunCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/AddCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/NewCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/ExecCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/DeployCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/PublishCommand.cs | Passes runtime selector to SDK install check |
| src/Aspire.Cli/Commands/PublishCommandBase.cs | Stores and validates runtime selector dependency |
| tests/Aspire.Cli.Tests/DotNet/DotNetRuntimeSelectorTests.cs | Tests for runtime selector initialization logic |
| tests/Aspire.Cli.Tests/Utils/ProcessLauncherTests.cs | Tests for process launcher |
| tests/Aspire.Cli.Tests/TestServices/TestDotNetRuntimeSelector.cs | Shared test double |
| tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs | Registers test runtime selector factory |
| tests/Aspire.Cli.Tests/Commands/SdkInstallerTests.cs | Updates tests to provide 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
+328
to
+337
| 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
+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
+430
to
+431
| // Get the correct dotnet executable path from the process launcher | ||
| var runtimeSelector = serviceProvider.GetRequiredService<IDotNetRuntimeSelector>(); |
Comment on lines
+301
to
+302
| using var httpClient = new HttpClient(); | ||
| var scriptContent = await httpClient.GetStringAsync(scriptUrl, cancellationToken); |
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18115Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18115" |
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
IDotNetRuntimeSelectorto ensure the correct runtime resolution logic is used consistently.Test plan
IDotNetRuntimeSelector-based availability check returns the correct result.Co-Authored-By: Claude noreply@anthropic.com