Skip to content

Stream project restore progress during winapp run - #789

Open
Nikola Metulev (nmetulev) wants to merge 15 commits into
mainfrom
nmetulev-improve-run-progress
Open

Stream project restore progress during winapp run#789
Nikola Metulev (nmetulev) wants to merge 15 commits into
mainfrom
nmetulev-improve-run-progress

Conversation

@nmetulev

@nmetulev Nikola Metulev (nmetulev) commented Aug 26, 2026

Copy link
Copy Markdown
Member

Summary

  • stream pre-build dotnet restore output, using dotnet's native terminal logger when interactive and raw line streaming when redirected
  • keep --quiet restores quiet, preserve JSON stdout, redact displayed commands, and avoid duplicate verbose command output
  • omit resolved Platform from solution-scoped restores so configuration-free .slnx files do not emit MSB4126
  • explain when best-effort restore failures fall back or continue, and preserve subprocess line formatting including blank lines
  • document the output behavior and add real .slnx sample coverage

Validation

  • 199 ProjectRunServiceTests passed in Release
  • real configuration-free WinUISolution.slnx run restored, built, and launched without MSB4126; the solution restore omitted -p:Platform while the project build retained it
  • plugin package validation and git diff --check passed

Environment note

The complete scripts\build-cli.ps1 run reached NativeAOT publish but could not finish on this device because the Visual Studio Desktop Development for C++ linker workload is not installed.

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Build Metrics Report

Binary Sizes

Artifact Baseline Current Delta
CLI (ARM64) 43.74 MB 43.72 MB 📉 -21.0 KB (-0.05%)
CLI (x64) 43.73 MB 43.71 MB 📉 -21.0 KB (-0.05%)
MSIX (ARM64) 18.11 MB 18.09 MB 📉 -23.7 KB (-0.13%)
MSIX (x64) 19.20 MB 19.19 MB 📉 -15.1 KB (-0.08%)
NPM Package 37.68 MB 37.67 MB 📉 -17.1 KB (-0.04%)
NuGet Package 37.81 MB 37.78 MB 📉 -34.0 KB (-0.09%)

Test Results

4777 passed, 5 skipped out of 4782 tests in 830.8s (+13 tests, -28.1s vs. baseline)

Test Coverage

89.4% line coverage, 82.5% branch coverage · ✅ +0.1% vs. baseline

CLI Startup Time

53ms median (x64, winapp --version) · 📉 -11ms vs. baseline

Try This Build

Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.

& ([scriptblock]::Create((irm https://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 789
Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://github.com/ghraw/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 789

Run winapp-pr with no arguments to pick from a list of open PRs.


Updated 2026-09-04 06:55:47 UTC · commit df4f9a1 · workflow run

@nmetulev
Nikola Metulev (nmetulev) marked this pull request as ready for review August 26, 2026 04:23
Copilot AI balanced review requested due to automatic review settings August 26, 2026 04:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Streams project restore progress alongside build output while preserving stdout for structured and quiet modes.

Changes:

  • Adds live restore streaming with terminal-aware output.
  • Expands restore-path tests and fake service support.
  • Documents restore and build output behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
ProjectRunService.cs Streams pre-build restore output.
ProjectRunServiceTests.cs Tests streamed and inherited restore paths.
FakeDotNetService.cs Simulates streaming output in tests.
docs/usage.md Documents project restore output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs
Comment thread docs/usage.md Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@nmetulev
Nikola Metulev (nmetulev) force-pushed the nmetulev-improve-run-progress branch from de57385 to 3c6fd63 Compare August 26, 2026 23:22

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The direction here is right, and on a classic .sln this works exactly as intended — restore progress streams, --no-restore is set correctly on the build, and --json stdout stays pure.

One blocker: on .slnx solutions this now surfaces an error MSB4126 on runs that succeed. It's a pre-existing latent failure that the old buffered call was hiding, so the streaming change is doing its job — but the failure itself needs fixing rather than displaying, since .slnx is what dotnet new sln produces by default on .NET 10 and every WinUI/Reactor template trips the condition. Found it running a Microsoft.UI.Reactor app in a two-project solution.

Also a smaller one: --quiet isn't quiet anymore for the restore pass.

Comment thread src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs
Comment thread src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs
Comment thread src/winapp-CLI/WinApp.Cli.Tests/FakeDotNetService.cs Outdated
Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Outdated
@zateutsch

Copy link
Copy Markdown
Contributor

🤖 AI-generated review (winappcli pr-review skill) — verify before acting.

PR Review — nmetulev-improve-run-progress vs origin/main

Decision

Changes required — the restore-progress change creates two user-visible regressions: --quiet now emits informational restore chatter, and best-effort restore failures print alarming NuGet errors even when the app subsequently builds and launches successfully.

Must fix

Quiet mode emits normal restore progress

  • What is wrong: Restore arguments do not apply quiet verbosity, so the new streaming path forwards informational dotnet restore output under --quiet.
  • Show me: winapp run Review.slnx --project App --quiet -p WindowsPackageType=None → stderr contains Determining projects to restore... and Restored ...; expected: warnings and errors only.
  • Why it matters: Quiet automation becomes noisy and violates the documented Warning-level behavior.
  • Smallest fix: Add -v quiet to restore invocations in quiet mode while retaining stderr routing for warnings and errors; add a focused argument/output test.
  • Location: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.Arguments.cs:23-58, src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:608-619

Ignored restore failures look fatal

  • What is wrong: Solution and sibling restores are explicitly best-effort, but their raw NuGet errors now stream without a visible message that winapp is continuing. A failed solution restore can also be followed by a failed sibling restore, printing the errors twice before a successful build.
  • Show me: Run a solution whose non-target sibling references an unavailable package → winapp prints error NU1101, retries the sibling and prints another error NU1101, then reports Build succeeded, launches the app, and exits 0.
  • Why it matters: Users see apparent fatal errors and may stop or troubleshoot a feed problem that did not block their requested app.
  • Smallest fix: After each nonzero best-effort restore, print a clear visible message that winapp is continuing and that the target build will report any blocking error; keep the existing fallback behavior.
  • Location: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:542-568, src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:577-594

Non-blocking

Quiet-mode documentation promises suppressed command lines

  • What is wrong: The usage page says quiet mode prints each restore/build invocation to stderr, but the implementation suppresses those invocations and only streams command output.
  • Show me: winapp run App.slnx --quiet → no dotnet restore ... or dotnet build ... line; expected from the documentation: each invocation appears first.
  • Why it matters: Users cannot rely on the documented quiet output when reproducing a failure.
  • Smallest fix: Clarify that quiet mode suppresses invocations while routing restore/build output to stderr; only JSON mode prints the invocation there.
  • Location: docs/usage.md:757-765

Redirected restore output is wrapped instead of streamed as plain lines

  • What is wrong: ansiConsole.WriteLine wraps redirected restore output to Spectre.Console’s profile width.
  • Show me: Redirect a restore containing a long path or feed URL → one dotnet diagnostic is split across multiple roughly 80-column lines; expected: the original append-only dotnet line.
  • Why it matters: CI parsers can miss diagnostics, and paths and URLs become difficult to copy.
  • Smallest fix: Under the existing write lock, write through ansiConsole.Profile.Out.Writer.WriteLine(line) as the build path already does.
  • Location: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:632-642

What was exercised

  • dotnet run --project src\winapp-CLI\WinApp.Cli.Tests\WinApp.Cli.Tests.csproj -c Debug -- --filter "FullyQualifiedName~ProjectRunServiceTests" — 191/191 passed.
  • NativeAOT publish for win-arm64 and direct invocation of the published winapp.exe — succeeded.
  • Quiet-mode run against a cold-cache two-project solution — exited 0 and reproduced informational restore output on stderr.
  • Best-effort failure run with an unavailable sibling package and isolated empty feed — printed repeated NU1101 errors, then built, launched, and exited 0.
  • Property command-injection attempt containing & cmd /c — no marker file was created.
  • .\scripts\build-cli.ps1 -SkipTests — native x64/arm64 builds and npm/NuGet packaging completed; generated files were left unchanged.
  • Not exercised: MSIX packaging — the build-tools download failed with the known corporate-network SSL restriction, which does not affect these project-run findings.

@zateutsch Zach Teutsch (zateutsch) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review attached above.

Honor quiet verbosity during project restores, avoid duplicate verbose invocations, and document JSON and quiet routing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use scoped disposal for the JSON and quiet restore stderr capture writers.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@nmetulev

Copy link
Copy Markdown
Member Author

Zach Teutsch (@zateutsch) Addressed the review findings in the latest commits: quiet restores now use -v quiet; every nonzero best-effort restore now prints an explicit fallback/continuation warning; redirected restore and build lines bypass Spectre wrapping; and the usage/setup docs reflect the actual JSON and quiet contracts. The branch also fixes the .slnx Platform failure and adds real configuration-free .slnx coverage. Release ProjectRunServiceTests pass 199/199.

Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Fixed
Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Fixed
Comment thread src/winapp-CLI/WinApp.Cli.Tests/ProjectRunServiceTests.cs Fixed
@zateutsch

Copy link
Copy Markdown
Contributor

🤖 AI-generated review (winappcli pr-review skill) — verify before acting.

PR Review — nmetulev-improve-run-progress vs origin/main

Decision

Changes required — the new whole-solution restore can produce assets for a different MSBuild platform than the subsequent --no-restore project build. The branch otherwise has appropriate scope, documentation, tests, and security handling.

Must fix

Platform-less solution restore can poison the no-restore build

  • What is wrong: After resolving Platform=x64 or ARM64, winapp restores the solution without that platform, then builds the selected project with the platform and --no-restore. Platform-conditioned packages or runtime identifiers are therefore absent from project.assets.json.
  • Show me: A solution containing a project with an x64-only PackageReference: winapp run App.slnx --arch x64 --detach → solution restore omits -p:Platform=x64, then project build adds it with --no-restore and fails with CS0103 or NETSDK1047; expected: the selected project restores under the same platform it builds with.
  • Why it matters: Valid WinUI solutions that worked before this branch can fail with misleading missing-package or missing-assets errors.
  • Smallest fix: Keep omitting Platform from solution restores to avoid MSB4126, but only set the selected build to NoRestore when no platform was resolved. When a platform exists, let the project build perform its normal project-scoped restore.
  • Location: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:181

Non-blocking

Quiet fallback warnings contradict the clean-stdout contract

  • What is wrong: The three new restore fallback messages use LogWarning; this logger routes warnings to stdout even under --quiet, while the updated documentation promises clean stdout.
  • Show me: winapp run App.slnx --quiet --detach -p Platform=x64 with a failing solution restore → stdout contains the fallback warning before the detached PID; expected: restore diagnostics remain on stderr.
  • Why it matters: Scripts capturing quiet stdout can receive an unexpected human-readable line.
  • Smallest fix: Write these restore-failure explanations to stderr in quiet mode while retaining normal warning behavior otherwise.
  • Location: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.cs:518,566,597

What was exercised

  • scripts\build-cli.ps1 — native and npm builds succeeded; the full run reached 4,635 tests, with 70 unrelated failures from missing internal NuGet-feed configuration and one ARM64 crash-dump fixture mismatch.
  • dotnet run --project src\winapp-CLI\WinApp.Cli.Tests\WinApp.Cli.Tests.csproj -c Debug --no-build -- --filter "FullyQualifiedName~ProjectRunServiceTests" — 199 passed, including JSON secret-redaction and stdout/stderr routing coverage.
  • NativeAOT dotnet publish plus direct winapp.exe --version — publish succeeded and the binary ran.
  • Synthetic .slnx and classic .sln projects — reproduced the platform-conditioned package/RID failure; allowing the selected project to restore fixed it, while forwarding Platform to either solution format reproduced MSB4126.
  • Security red-team path: JSON restore with NuGetApiKey=top-secret — stderr contained NuGetApiKey=***, stdout stayed clean, and the secret was absent.
  • Not exercised: the full WinUI sample Pester workflow — the focused runtime reproductions covered the changed restore behavior, but not the sample’s complete packaging flow.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b54e905a-d32e-4e13-8c6d-4da7f9d80335
@nmetulev
Nikola Metulev (nmetulev) requested a balanced review from Copilot September 4, 2026 05:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation preserves output-mode contracts and includes focused unit and real-sample regression coverage.

Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b54e905a-d32e-4e13-8c6d-4da7f9d80335
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants