Defer dotnet restore in aspire update so channel switches don't trip NU1103#17017
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 17017Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 17017" |
There was a problem hiding this comment.
Pull request overview
This PR fixes a failure mode in aspire update when switching to an Explicit channel (daily/staging/PR hives/local) where nuget.config is rewritten (including packageSourceMapping) before packages are updated. The update previously performed per-package restores, which could run NuGet against a half-updated reference graph and fail with NU1103, leaving the project in a broken intermediate state.
Changes:
- Defers restore by switching per-package
dotnet package addcalls to--no-restore, then running a single finaldotnet restoreafter all edits are applied. - Adds unit coverage to enforce call ordering (
AddPackageAsync(... noRestore: true)for every package, and exactly oneRestoreAsyncexecuted last). - Adds an end-to-end regression test reproducing the original scenario against LocalHive, and introduces new localized resource strings for the final restore status/failure messaging.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/TestServices/TestDotNetCliRunner.cs | Makes RestoreAsync default to success so existing tests don’t start failing now that restore is always invoked. |
| tests/Aspire.Cli.Tests/Projects/ProjectUpdaterTests.cs | Tightens existing assertions to require NoRestore == true and adds a dedicated regression test asserting restore ordering. |
| tests/Aspire.Cli.EndToEnd.Tests/UpdateChannelNuGetConfigOrderingTests.cs | Adds an E2E regression test verifying nuget.config source mapping is applied and all #:package directives are updated before the single restore. |
| src/Aspire.Cli/Projects/ProjectUpdater.cs | Implements deferred restore behavior: per-package AddPackageAsync uses noRestore: true, then performs one RestoreAsync at the end with status/error handling. |
| src/Aspire.Cli/Resources/UpdateCommandStrings.resx | Adds new strings for “Restoring packages…” and restore-failure messaging. |
| src/Aspire.Cli/Resources/UpdateCommandStrings.Designer.cs | Regenerates the strongly-typed resource accessors for the new strings. |
| src/Aspire.Cli/Resources/xlf/UpdateCommandStrings.*.xlf | Updates localized XLF files with the new resource entries (state new). |
Copilot's findings
Files not reviewed (1)
- src/Aspire.Cli/Resources/UpdateCommandStrings.Designer.cs: Language not supported
- Files reviewed: 18/19 changed files
- Comments generated: 0
24310f0 to
b4a0cb6
Compare
|
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.
|
…NU1103 Fixes #15891. Before this change, `aspire update --channel daily` (or any other Explicit channel) on an AppHost referencing several stable Aspire integration packages would fail with NU1103 partway through. The CLI rewrote nuget.config to add the channel's feed plus a `<packageSourceMapping>` pinning `Aspire*` to that feed, then ran each per-package `dotnet package add` with restore enabled. The first add's restore saw the still-stable references for the not-yet-bumped packages, but the new mapping blocked the nuget.org fallback for them, so NuGet emitted NU1103 and the update aborted halfway through — leaving the project in a broken intermediate state (new nuget.config, only the first package bumped). Fix in `ProjectUpdater`: - Pass `noRestore: true` on every per-package `AddPackageAsync` so each `dotnet package add` only mutates the project / file-based AppHost. - After the existing "Applying updates" loop completes, run a single `runner.RestoreAsync(...)` so users still get a clear failure surface if anything else is misconfigured. Verified locally on .NET SDK 10.0.203 that `dotnet restore <path-to-apphost.cs>` works for both csproj and file-based AppHosts — no new runner abstraction needed. New `RestoringPackagesAfterUpdate` / `FailedToRestoreAfterUpdateFormat` resource strings carry the status and error messaging; xlf files regenerated via `dotnet build /t:UpdateXlf`. Tests, in three layers so regressions are caught at PR time and in CI: 1. New unit test `UpdateProjectFileAsync_AppliesAllPackageEditsBeforeFinalRestore` in `ProjectUpdaterTests` wires `AddPackageAsync` and `RestoreAsync` to a shared call-order list and asserts every add carries `noRestore: true`, exactly one restore is invoked, and it runs last. 2. The five existing `AddPackageAsync` capture sites in `ProjectUpdaterTests` now also assert `item.NoRestore == true` (and the misleading "should be null because of --no-restore behavior" comment that actually referred to `nugetSource` is corrected). Catches regressions on every existing scenario — traditional PackageReference, multi-project, single-file AppHost, mixed paths. `TestDotNetCliRunner.RestoreAsync` now defaults to returning 0 instead of throwing `NotImplementedException`, mirroring the `AddProjectToSolutionAsync` / `GetNuGetConfigPathsAsync` pattern. Required because `UpdateProjectAsync` always calls `RestoreAsync` now. 3. New CLI E2E test `UpdateChannelNuGetConfigOrderingTests.AspireUpdateAppliesAllPackageEditsBeforeRestoringWhenNuGetConfigGainsSourceMapping` in its own test class so CI's per-class job split (`SplitTestsOnCI`) puts the regression on its own runner. Exercises the bug end-to-end against LocalHive: writes an `apphost.cs` with three stable `Aspire.Hosting.*` references and a nuget.org-only `nuget.config`, runs `aspire update --channel local`, and asserts the merged `packageSourceMapping` actually landed (so we know the bug pre-condition was triggered) and every `#:package` directive was bumped (so we know the loop ran to completion rather than aborting after the first add). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
b4a0cb6 to
4a6bf4b
Compare
…file AppHosts The Aspire.AppHost.Sdk pulls Aspire.Hosting.AppHost in implicitly, both for the new csproj SDK format and for single-file (apphost.cs) AppHosts. The csproj path already removes a redundant explicit PackageReference, but the .cs path left an explicit '#:package Aspire.Hosting.AppHost@<ver>' directive in place. After 'aspire update --channel daily' rewrites nuget.config to pin Aspire* to the channel feed, the deferred restore then fails with NU1103 because the channel does not carry the user's stable version of Aspire.Hosting.AppHost. Mirror the csproj cleanup for single-file AppHosts: strip the directive inline during the SDK-bump path, and enqueue a cleanup step on the SDK-already-current path so a re-run after a partial migration recovers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎬 CLI E2E Test Recordings — 82 recordings uploaded (commit View all recordings
📹 Recordings uploaded automatically from CI run #25899938835 |
The aspire update command now applies all package version edits before running a single final dotnet restore, preventing NU1103 failures when switching to channels that add NuGet source mappings (daily, staging, etc.) Fixes microsoft/aspire#17017 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Pull request created: #989
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#989 targeting Updated
Note This draft PR needs human review before merging. |
Fixes #15891.
What the user sees
aspire update --channel daily(or any other Explicit channel — staging, PRhives, local) on a file-based AppHost referencing several stable Aspire
integration packages fails partway through with:
The project is left in a broken intermediate state: new
nuget.config(nowrestricting
Aspire*to the daily feed), only the first package bumped to adaily version, the rest still on the stable version that no longer resolves.
Root cause
ProjectUpdater.UpdateProjectAsyncran in this order:nuget.configvia
NuGetConfigMerger.CreateOrUpdateAsync— adds the<packageSourceMapping>pinningAspire*to the channel's feed.runner.AddPackageAsync(..., noRestore: false, ...)— i.e. eachdotnet package addran its own restore.The first restore happened with
nuget.configalready in "channel-only" modebut the rest of the
PackageReferences still on stable versions. With<packageSourceMapping>blocking nuget.org forAspire*, NuGet emittedNU1103for the not-yet-bumped Aspire packages and the update aborted.Fix
Defer the implicit restore until after every package version edit has been
applied:
UpdatePackageReferenceInProjectnow passesnoRestore: trueon everyper-package
AddPackageAsyncso eachdotnet package addonly mutates theproject / file-based AppHost.
UpdateProjectAsyncruns a singlerunner.RestoreAsync(...)so users stillget a clear failure surface if anything else is misconfigured.
By the time NuGet sees the new
<packageSourceMapping>, everyAspire*reference already points at a version that exists in the channel's feed, so
the mapping no longer causes
NU1103.Verified locally on .NET SDK 10.0.203 that
dotnet restore <path-to-apphost.cs>works for both
.csprojand file-based AppHosts — the existingIDotNetCliRunner.RestoreAsyncshape covers both with no signature change.New
RestoringPackagesAfterUpdate/FailedToRestoreAfterUpdateFormatresource strings carry the status and error messaging; xlf files regenerated
via
dotnet build /t:UpdateXlf.Tests
Three layers, each catching a different regression class:
Unit (PR-time guard, fast) —
ProjectUpdaterTests.UpdateProjectFileAsync_AppliesAllPackageEditsBeforeFinalRestorewires
AddPackageAsyncandRestoreAsyncto a shared call-order list andasserts every add carries
noRestore: true, exactly one restore isinvoked, and it runs last. If anyone flips
noRestoreback tofalseor moves the restore step before the loop, this fails immediately.
Tightened existing tests — the five
AddPackageAsynccapture sites inProjectUpdaterTestsnow also assertitem.NoRestore == true(and themisleading "should be null because of
--no-restorebehavior" comment thatactually referred to
nugetSourceis corrected). Catches regressions onevery existing scenario — traditional
PackageReference, multi-project,single-file AppHost, mixed paths.
End-to-end CLI test (CI guard, real bug repro) —
UpdateChannelNuGetConfigOrderingTests.AspireUpdateAppliesAllPackageEditsBeforeRestoringWhenNuGetConfigGainsSourceMapping,in its own test class so CI's per-class job split (
SplitTestsOnCI=true)puts it on its own runner. Exercises the bug end-to-end against LocalHive:
writes an
apphost.cswith three stableAspire.Hosting.*references(
Redis,PostgreSQL,Kafkaat13.2.1— the same release used in theissue's repro) and a nuget.org-only
nuget.config, runsaspire update --channel local, then asserts:<packageSourceMapping>actually landed (so we know the bugpre-condition was triggered, not silently bypassed), and
#:packagedirective was bumped to the local SDK version (so weknow the loop ran to completion rather than aborting after the first add).
Skips on non-LocalHive strategies because reproducing against the public
daily feed isn't deterministic.
Notes for reviewers
TestDotNetCliRunner.RestoreAsyncnow defaults to returning0instead ofthrowing
NotImplementedException, mirroring the existingAddProjectToSolutionAsync/GetNuGetConfigPathsAsyncpatterns. Requiredbecause
UpdateProjectAsyncalways callsRestoreAsyncnow and mostexisting
ProjectUpdatertests don't set the callback. No existing testrelied on the throw behavior.
UpdatePackageVersionInDirectoryPackagesProps) editsDirectory.Packages.propsdirectly and never invoked restore on its own,so it was never part of the bug — but the new single deferred restore
covers it too.
UpdateProjectFileAsync_CanUpdateFromStableToDailyalreadyexercises the Explicit channel path, so the production fix is hit by
multiple tests, not just the new one.