Avoid running MSBuild during aspire ls AppHost discovery in the VS Code extension#18348
Draft
ellahathaway wants to merge 10 commits into
Draft
Avoid running MSBuild during aspire ls AppHost discovery in the VS Code extension#18348ellahathaway wants to merge 10 commits into
aspire ls AppHost discovery in the VS Code extension#18348ellahathaway wants to merge 10 commits into
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18348Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18348" |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a --no-evaluate mode to aspire ls AppHost discovery so the VS Code extension can populate its tree quickly without triggering a design-time MSBuild "storm" (one evaluation per candidate project). In this mode the CLI is cache-first: it returns already-known AppHost details when present, and otherwise emits a best-effort heuristic signal (possibly-buildable) instead of evaluating. The extension detects the new ls-no-evaluate.v1 capability via the shared ConfigInfoProvider, opts into the flag when supported, and gracefully degrades on older CLIs. It fixes #18175.
Changes:
- CLI: thread a
noEvaluateflag throughProjectLocator,AppHostInfoResolver, andIAppHostProject.ValidateAppHostAsync; add thePossiblyBuildablecandidate status, thels-no-evaluate.v1capability, and a content-keyed disk cache path normalization; makeAppHostProjectInfo.IsAspireHostnullable to represent "not evaluated." - Extension: centralize CLI status vocabulary in
appHostStatus.ts, add a neutralappHostCandidate.tstype, render candidate status (icon +Status:row), and refine candidate refresh/warn-on-build-failure behavior. - Docs, localization (resx/Designer/xlf), and unit/E2E test coverage for the new mode and status.
Reviewed changes
Copilot reviewed 72 out of 74 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Cli/Caching/AppHostInfoDiskCache.cs |
New NormalizePathForHash for case-insensitive cache keys (macOS gap — see comment). |
src/Aspire.Cli/Projects/AppHostInfoResolver.cs |
Cache-first heuristic fallback (CreateHeuristicInfo) and nullable IsAspireHost. |
src/Aspire.Cli/Projects/DotNetAppHostProject.cs |
ValidateAppHostAsync(noEvaluate) returning IsNotEvaluated on null (uses == null — see comment). |
src/Aspire.Cli/Projects/ProjectLocator.cs |
Threads noEvaluate; maps IsNotEvaluated → PossiblyBuildable; new enum value + warning. |
src/Aspire.Cli/Commands/LsCommand.cs |
--no-evaluate option, status serialization/markup. |
extension/src/utils/appHostStatus.ts (new), types/appHostCandidate.ts (new) |
Status vocabulary + neutral candidate type. |
extension/src/utils/appHostDiscovery.ts, views/AspireAppHostTreeProvider.ts, views/AppHostDataRepository.ts, services/AppHostLaunchService.ts |
Capability opt-in, status rendering, candidate refresh/warn behavior. |
docs/specs/cli-output-formats.md |
Documents --no-evaluate and possibly-buildable. |
Localization (*.resx, *.Designer.cs, xlf/*) |
New option/error strings + generated translations. |
tests/Aspire.Cli.Tests/**, extension/src/test*/** |
Unit/E2E coverage for cache-first behavior and new status. |
Notable findings:
- Critical bug:
NormalizePathForHash(new in this PR —mainappended the rawFullName) only upper-cases the Windows drive letter, yet its comment claims macOS is also case-insensitive. On macOS it returns the path unchanged, so the newComputeKey_IsCaseInsensitiveForProjectPathOnCaseInsensitiveFilesystemstest (which runs on macOS) fails, and the cache-hit feature is defeated on macOS for differently-cased paths. - Nit:
DotNetAppHostProject.csline 211 uses== nullwhere the file (and AGENTS.md) consistently useis null.
Files not reviewed (2)
- src/Aspire.Cli/Resources/ErrorStrings.Designer.cs: Generated file
- src/Aspire.Cli/Resources/SharedCommandStrings.Designer.cs: Generated file
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
The test case-folded the entire project path and ran on macOS, but NormalizePathForHash only normalizes the Windows drive letter, so the test failed on Windows. Scope the test to Windows, flip only the drive letter, and trim the NormalizePathForHash comment to match the actual behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 74 changed files in this pull request and generated 3 comments.
Files not reviewed (2)
- src/Aspire.Cli/Resources/ErrorStrings.Designer.cs: Generated file
- src/Aspire.Cli/Resources/SharedCommandStrings.Designer.cs: Generated file
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Replace brittle spawn call-order assumptions with command-aware stubbing in appHostDataRepository tests, add async waits for context transitions, and add missing repository refresh stubs in appHostTreeView tests so launch-error assertions validate intended failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 75 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- src/Aspire.Cli/Resources/ErrorStrings.Designer.cs: Generated file
- src/Aspire.Cli/Resources/SharedCommandStrings.Designer.cs: Generated file
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 75 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- src/Aspire.Cli/Resources/ErrorStrings.Designer.cs: Generated file
- src/Aspire.Cli/Resources/SharedCommandStrings.Designer.cs: Generated file
|
|
||
| /** | ||
| * AppHost candidate statuses emitted by `aspire ls`. The CLI owns this vocabulary | ||
| * (see `AppHostProjectCandidateStatus` in src/Aspire.Cli/Commands/LsCommand.cs), so the wire |
Contributor
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. Test projects (2 / 97)
Jobs (6)
Selection computed for commit |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
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.
Fixes #18175.
Problem
When the VS Code extension discovers AppHosts in a workspace, it calls
aspire ls --format json. To classify each candidate, the CLI ran a design-time MSBuild evaluation of every matching project. In large workspaces (or on cold start) this produced an "MSBuild storm" — many concurrent evaluations that made discovery slow and CPU-heavy, even though the extension only needs a fast, best-effort list to populate the tree.Solution
Add a
--no-evaluatemode toaspire lsand have the extension opt into it. In this mode discovery is cache-first and never triggers a fresh evaluation: it uses already-known AppHost details when present, and otherwise reports a best-effort guess from project naming/layout cues. The extension treats those unverified-but-likely candidates as launchable.CLI (
src/Aspire.Cli)aspire ls --no-evaluateoption. Threaded throughProjectLocator,AppHostInfoResolver, andIAppHostProject.ValidateAppHostAsyncso a cache miss returns a heuristic signal instead of invoking MSBuild.possibly-buildable(alongsidebuildableandpossibly-unbuildable) for AppHosts that look launchable but were not evaluated. Documented indocs/specs/cli-output-formats.md.ls-no-evaluate.v1advertised throughaspire config infoso tooling can detect support before passing the flag..xlfupdates).VS Code extension (
extension/)ls-no-evaluate.v1via the sharedConfigInfoProvider(no extraconfig infocall) and appends--no-evaluateonly when supported. Backwards compatible: an older CLI degrades to plainaspire ls, and then to the legacyaspire extension get-apphostsfallback.appHostStatus.tscentralizes the CLI status vocabulary — buildable rule (buildable+possibly-buildableare launchable), user-facing label, and tree icon — so they can't drift apart. New neutraltypes/appHostCandidate.tskeepsstatusastringso a newer CLI value can't break deserialization.Status: …row).Testing
LsCommandTests,AppHostInfoResolverTests,DotNetAppHostProjectTests,ProjectLocatorTests,AppHostInfoDiskCacheTestscover--no-evaluatecache-first behavior and the new status.appHostDiscovery.test.tsverifies--no-evaluateis appended when the capability is advertised and omitted (falling back to plainaspire ls) when it isn't;appHostTreeView.test.ts/appHostDataRepository.test.tscover status rendering and candidate state.appHostTree.e2e.test.ts): promotion of apossibly-buildableidle candidate tobuildable, warn-and-remove when a build actually fails, and reclassification on debug-session-end.tsc+eslintclean; the new--no-evaluatediscovery tests pass.Video demo
no-evaluate.mp4