Fix CLI bundle extraction to use ~/.aspire/ for non-standard install paths#15563
Fix CLI bundle extraction to use ~/.aspire/ for non-standard install paths#15563mitchdenny wants to merge 5 commits intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15563Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15563" |
|
/deployment-test |
|
🚀 Deployment tests starting on PR #15563... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
There was a problem hiding this comment.
Pull request overview
This PR fixes bundle extraction/layout discovery when the Aspire CLI binary is installed outside the standard ~/.aspire/bin/ layout (e.g., Homebrew/Winget), ensuring extraction artifacts land in the user-writable ~/.aspire/ directory instead of potentially system-owned parent directories.
Changes:
- Update bundle extraction default path selection to detect the standard
.aspire/binlayout and otherwise fall back to the well-known~/.aspire/directory. - Extend layout discovery to also probe the well-known
~/.aspire/path after env-var and relative-path checks. - Add/adjust unit tests for extraction path logic and layout discovery behavior; document the resolution rules.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/LayoutDiscoveryTests.cs | New tests validating env var precedence and rejecting invalid env-var layouts. |
| tests/Aspire.Cli.Tests/BundleServiceTests.cs | Updates/expands tests for standard vs non-standard install locations and well-known path computation. |
| src/Aspire.Cli/Layout/LayoutDiscovery.cs | Adds well-known ~/.aspire/ fallback discovery step. |
| src/Aspire.Cli/Bundles/BundleService.cs | Changes default extraction directory logic; introduces well-known path helper and .aspire directory check. |
| docs/specs/bundle.md | Documents extraction directory resolution and matching discovery order. |
| @@ -167,7 +169,39 @@ private async Task<BundleExtractResult> ExtractCoreAsync(string destinationPath, | |||
| return null; | |||
There was a problem hiding this comment.
GetDefaultExtractDir returns null when processPath has no directory component (e.g., a relative/filename-only path). That contradicts the method’s documented intent to fall back to the well-known ~/.aspire/ directory for non-standard locations, and it can cause EnsureExtractedAsync to skip extraction or aspire setup to fail with "Could not determine the installation path." Consider returning GetWellKnownAspireDir() instead of null when cliDir is null/empty.
| return null; | |
| return GetWellKnownAspireDir(); |
|
❌ Deployment E2E Tests failed — 25 passed, 3 failed, 0 cancelled View test results and recordings
|
| /// </summary> | ||
| internal static string GetWellKnownAspireDir() | ||
| { | ||
| return Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), ".aspire"); |
There was a problem hiding this comment.
Can this be reused from somewhere? We must have code to generate this path somewhere else.
| private static bool IsAspireDirectory(string directoryPath) | ||
| { | ||
| var dirName = Path.GetFileName(directoryPath); | ||
| return string.Equals(dirName, ".aspire", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
What happens if someone specifies an install directory of C:\Program Files\WinGet\Links\.aspire\. Won't this return true and produce a bad result?
| var fakeLayoutDir = CreateTempDirectory(); | ||
| CreateValidBundleLayout(fakeLayoutDir); | ||
|
|
||
| var envBefore = Environment.GetEnvironmentVariable(BundleDiscovery.LayoutPathEnvVar); |
There was a problem hiding this comment.
These tests modify global env var state. Flaky.
| return null; | ||
| } | ||
|
|
||
| return Path.GetDirectoryName(cliDir) ?? cliDir; | ||
| var parentDir = Path.GetDirectoryName(cliDir); | ||
| if (parentDir is null) | ||
| { | ||
| return GetWellKnownAspireDir(); | ||
| } | ||
|
|
||
| // If the parent directory is the well-known .aspire directory (standard layout), | ||
| // use it directly so extraction lands alongside the CLI. |
There was a problem hiding this comment.
The fallback behavior is inconsistent between degenerate inputs:
- When
Path.GetDirectoryName(processPath)is empty (e.g., bare"aspire"with no directory) → returnsnull, which causes callers to skip extraction entirely. - When
cliDirexists butPath.GetDirectoryName(cliDir)is null (binary at filesystem root, e.g.,/aspire) → returnsGetWellKnownAspireDir(), which silently extracts to~/.aspire/.
Both represent "we can't determine a meaningful install location from the process path." Should these be treated the same way? I'd expect either both to return null (skip extraction) or both to fall back to the well-known dir.
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
Overall this is a solid fix for the non-standard install path problem. Four issues to address.
|
This is a release/13.2 candidate. The goal is to get this out so that we can ship winget/brew installers. |
…paths When the Aspire CLI is installed outside the standard ~/.aspire/bin/ layout (e.g., via Homebrew at /usr/local/bin/ or Winget at C:\Program Files\), the bundle extraction previously wrote .aspire-bundle-version and extracted managed/dcp directories to the parent of the CLI's parent directory, which could be a system directory like /usr/local/ or C:\Program Files\. This change: - Updates GetDefaultExtractDir to detect non-standard install locations and fall back to the well-known ~/.aspire/ directory - Adds ~/.aspire/ as a well-known layout discovery path in LayoutDiscovery so the CLI can find extracted content when installed elsewhere - Adds tests for the new path resolution logic - Documents the extraction directory resolution in the bundle spec Fixes #15454 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add AspireDirectory property to CliExecutionContext - Inject CliExecutionContext into BundleService and LayoutDiscovery - Remove static GetWellKnownAspireDir() and IsAspireDirectory() helpers - Use exact path comparison against context.AspireDirectory instead of fragile directory name check - Drop unnecessary null guards (internal API, always receives valid path) - Rewrite tests to use CliExecutionContext instead of env var mutations - Add GetDefaultExtractDir to IBundleService interface Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…text The LayoutDiscovery class is public but CliExecutionContext is internal, so DI reflection cannot locate a public constructor. Use a factory registration to construct LayoutDiscovery explicitly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ove dead code, use CliExecutionContext for env vars - Return Path.GetFullPath(parentDir) in GetDefaultExtractDir for consistent output - Derive AspireDirectory from HomeDirectory to avoid duplicated fallback logic - Remove unreachable string.IsNullOrEmpty(installPath) check in SetupCommand - Replace all direct Environment.GetEnvironmentVariable calls in LayoutDiscovery with _executionContext.GetEnvironmentVariable() for testability - Pass empty environmentVariables dict in LayoutDiscoveryTests for isolation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mitchdenny
force-pushed
the
fix/bundle-version-path
branch
from
044f19c to
7e055d4
Compare
mitchdenny
requested review from
adamint,
danegsta,
eerhardt,
karolz-ms and
sebastienros
as code owners
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 52 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23620894923 |
|
|
||
| public LayoutConfiguration? DiscoverLayout(string? projectDirectory = null) | ||
| { | ||
| // 1. Try environment variable for layout path |
There was a problem hiding this comment.
LayoutDiscovery is a public sealed class but the constructor is now internal. Since CliExecutionContext is internal, the constructor can't be public — but this leaves a public type with no accessible constructor. Consider making LayoutDiscovery itself internal to match.
There was a problem hiding this comment.
This doesn't matter because this is the CLI. But LayoutDiscovery class itself should be internal. If it's changed to internal then I think the constructor can then be made public and then you don't need to manually create the type with its resolved dependencies in a DI callback.
| // it should NOT find one at the well-known path because it's incomplete | ||
| if (layout is not null) | ||
| { | ||
| Assert.NotEqual(aspireDir, layout.LayoutPath); |
There was a problem hiding this comment.
These tests have weak assertions — if layout is null, no assertion runs and the test passes vacuously. The test name promises "ReturnsNull" / "RejectsLayout" but doesn't actually enforce that. The reason is that TryDiscoverRelativeLayout() still uses Environment.ProcessPath directly (see other comment), so the test can't predict whether a layout will be found via the relative path probe.
At minimum, Assert.Null(layout) would be the correct assertion here if you make ProcessPath controllable via the execution context.
| _logger.LogDebug("Discovered layout at well-known path: {Path}", wellKnownLayout.LayoutPath); | ||
| return LogEnvironmentOverrides(wellKnownLayout); | ||
| } | ||
|
|
There was a problem hiding this comment.
This still reads Environment.ProcessPath directly instead of going through _executionContext. All the Environment.GetEnvironmentVariable calls were migrated to use the execution context, but this one was missed.
This makes LayoutDiscovery impossible to fully test in isolation — LayoutDiscoveryTests can't control ProcessPath, which is why the tests above need the weak if (layout is not null) guard instead of asserting Assert.Null(layout).
Consider adding a ProcessPath property to CliExecutionContext so this can be injected in tests too.
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
A few cleanup things to address and then good to merge.
Description
Fixes #15454
When the Aspire CLI is installed outside the standard
~/.aspire/bin/layout (e.g., via Homebrew at/usr/local/bin/or Winget atC:\Program Files\), the bundle extraction previously wrote.aspire-bundle-versionand extractedmanaged//dcp/directories to the parent of the CLI's parent directory — which could be a system directory like/usr/local/orC:\Program Files\.This PR changes the default extraction directory logic to detect non-standard install locations and fall back to the well-known
~/.aspire/directory instead.Changes
BundleService.csGetDefaultExtractDirto check if the parent-of-parent is a.aspiredirectory (standard layout). If not, falls back to~/.aspire/.GetWellKnownAspireDir()helper that returns~/.aspire/cross-platform.IsAspireDirectory()helper for the directory name check.LayoutDiscovery.cs~/.aspire/as a well-known fallback inDiscoverLayout()(step 3, after env var and relative-path checks).~/.aspire/bin/.BundleServiceTests.csGetDefaultExtractDir_ReturnsAspireDir_ForStandardLayout.GetDefaultExtractDir_FallsBackToWellKnownDir_ForNonStandardLayout(Homebrew, Winget).GetDefaultExtractDir_FallsBackToWellKnownDir_ForCustomInstallLocation.GetWellKnownAspireDir_ReturnsExpectedPath.LayoutDiscoveryTests.cs(new)docs/specs/bundle.mdBackward Compatibility
For the standard install path (
~/.aspire/bin/aspire), behavior is unchanged —GetDefaultExtractDirreturns~/.aspire/in both the old and new code paths. The--install-pathflag onaspire setupcontinues to override all default logic.