Hide MSBuild property JSON from winapp run --aot output - #936
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Nikola Metulev (nmetulev)
added
the
agent-preparing
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is focused and the build plus 69 targeted tests passed.
Review effort: Balanced
Findings: None
What changed in this PR
Suppresses MSBuild property JSON from winapp run --aot while preserving diagnostics and property-based output resolution.
Changes:
- Buffers publish stdout and removes only the final property envelope.
- Preserves live stderr and other publish diagnostics.
- Adds regression tests and updates usage documentation.
| File | Description |
|---|---|
ProjectRunService.Aot.cs |
Filters buffered publish output. |
MsBuildPropertyReader.cs |
Locates and removes the final property envelope. |
ProjectRunServiceAotTests.cs |
Tests success, failure, quiet, and JSON output. |
MsBuildPropertyReaderTests.cs |
Tests filtering and diagnostic preservation. |
docs/usage.md |
Documents revised AOT output behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Build Metrics ReportValidation passed. All required build and validation jobs succeeded. Binary Sizes
.NET Test Results (TRX reports)Other suites are reflected in the overall validation status above. ✅ 7879 passed, 37 skipped out of 7916 tests in 1114.2s (-81.5s vs. baseline) Test Coverage✅ 86.3% line coverage, 80.9% branch coverage · ✅ no change vs. baseline CLI Startup Time58ms median (x64, Try This BuildInstalls 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://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 936Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 936Run Updated 2026-09-28 22:26:15 UTC · commit |
Nikola Metulev (nmetulev)
added
ready-for-review
Zach Teutsch (zateutsch)
left a comment
Zach Teutsch (zateutsch)
left a comment
There was a problem hiding this comment.
🤖 AI-generated review (winappcli pr-review skill) — verify before acting.
Summary
The change produces correct output and CI is green, but there's a materially simpler approach that avoids both the stdout-buffering behavior change and the new offset-tracking in the shared property parser. Raising as changes requested so we can weigh it before merge.
Must fix — redirect the property JSON to a file instead of buffering stdout and cutting it out
What the PR does today: to hide MSBuild's final { "Properties": {...} } envelope, it (a) switches publish stdout from live to buffered (onOutputLine: null) and replays it after publish exits, and (b) adds character start/length tracking to the shared ScanJsonObjects / FindLastPropertiesObject so it can stdout.Remove(start, length).
Why reconsider: MSBuild can write the property result straight to a file, so neither piece is needed.
dotnet publish --getProperty:TargetPath --getResultOutputFile:props.jsonon a scratch app → stdout keeps its normal build diagnostics (Determining projects to restore… app -> …\publish\) and the property result lands inprops.json, off stdout entirely.- With the current approach, that same stdout — including target output/warnings, which the
…ShowsEarlierTargetOutputtest exercises — is withheld until the publish process exits, so a multi-minute Native AOT publish can look hung.
Why it matters: using the file redirect removes a real behavior change (loss of live stdout during a long publish) and lets us delete the offset math from MsBuildPropertyReader, a helper shared by all property parsing — less surface to get wrong, for the same user-visible result.
Smallest fix: add a per-invocation --getResultOutputFile:<temp> to the AOT publish args, keep stdout+stderr streaming as before, parse properties from that file, and delete it in a finally. Then drop WithoutLastPropertiesObject, the offset parameters on ScanJsonObjects, and revert RunPublishPassAsync to the streaming call. Please confirm --getResultOutputFile is available on the project's supported SDK floor — it's MSBuild-forwarded and present in the dotnet 10 build SDK here.
Locations: src/winapp-CLI/WinApp.Cli/Services/ProjectRunService.Aot.cs (RunPublishPassAsync) and src/winapp-CLI/WinApp.Cli/Helpers/MsBuildPropertyReader.cs.
Non-blocking — shipped setup skill says AOT output "streams live"
plugins/winapp/skills/winapp-setup/SKILL.md (Output bullet) says winapp "streams both commands' output live" in the section covering winapp run . --aot. The buffering approach makes that inaccurate for AOT publish stdout. This becomes moot if you adopt --getResultOutputFile (stdout streams live again) — in which case the reworded docs/usage.md line would also need to drop "shown after publishing completes." If you keep buffering, qualify this bullet for --aot.
What I checked
dotnet publish --getProperty:… [--getResultOutputFile:…]on a scratch console app (dotnet 10.0.401) — confirmed the file redirect keeps the property JSON off stdout while restoring live build diagnostics.- The UTF-8/UTF-16 offset math in the parser is correct for multibyte/surrogate content, so there's no correctness bug there. Security: nothing found. The one concern is the approach itself.
- Not exercised: a full
winappbinary run against a real Native AOT project (needs the AOT toolchain + a multi-minute build), so the exact magnitude of the buffering delay is inferred from the code path rather than measured.
Nikola Metulev (nmetulev)
added
agent-preparing
Address review feedback: instead of buffering publish stdout and cutting the final --getProperty envelope out of it, pass --getResultOutputFile so MSBuild writes the property result to a temp file. Publish output streams live again, and the shared MsBuildPropertyReader is unchanged from main. --getResultOutputFile ships in MSBuild 17.10, so --aot now needs .NET SDK 8.0.300 or newer; docs and the setup skill say so. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Zach Teutsch (@zateutsch) thanks, good call. Switched to
Verified with a real packaged WinUI ARM64 |
Nikola Metulev (nmetulev)
added
ready-for-review
Description
winapp run --aotprinted MSBuild's large--getPropertyresult in the terminal after publishing. Winapp now passes--getResultOutputFileso MSBuild writes that result to a temporary file, which winapp reads to resolve the executable and package, then deletes. Publish output still streams live, and the JSON no longer appears.--getResultOutputFileships in MSBuild 17.10, so--aotnow requires .NET SDK 8.0.300 or newer.Usage Example
Before: the publish status was followed by a large
{ "Properties": { ... } }block. After: normaldotnet publishprogress streams live, followed byNative AOT output: ...and launch status.Related Issue
N/A
Type of Change
Checklist
--json/--quietdocs/usage.mdand thewinapp-setupskill updatedScreenshots / Demo
Observed output from a local packaged WinUI ARM64 run (paths abbreviated):
Additional Notes
MsBuildPropertyReaderis unchanged frommain.ProjectRunService/MsBuildPropertyReader/RunCommandtests passed;scripts\build-cli.ps1 -SkipTests -SkipMsix -SkipNpm -SkipNuGetsucceeded. The WinUI run used a%TEMP%path containing a space, and no result file was left behind.build-and-package,test-samples-result, and all sample and validation jobs passed on this head.