Bubble up child process errors in IndexCreationTool#6261
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| "Input": "%BUILD_SOURCESDIRECTORY%/src/AppInstallerCLIE2ETests/TestData/AppInstallerTestFont.ttf", | ||
| "HashToken": "<FONTHASH>" | ||
| "HashToken": "<FONTHASH>", | ||
| "SkipSignature": true |
There was a problem hiding this comment.
signtool can't sign fonts without a SIP and font-specific key formats. my change bubbled up the error and caused test failures, but I believe this is the only one. couldn't get e2e tests fully working locally though
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Signed-off-by: Tom Plant <tom.plant@devicie.com>
pl4nty
force-pushed
the
bubble-IndexCreationTool-errors
branch
from
c9951d1 to
2dc8196
Compare
Not supported by signtool without SIP etc
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I've spent a while trying to reproduce these E2E test failures locally, but haven't been able to. Could they just be flaky? They seem stable in the ADO reports, but I was surprised that x86 failed when x64 succeeded. Maybe it's worth retrying after #6354 is merged? |
It appears to be the ADO image version; x86 ran with the newer one and x64 the older one. |
There was a problem hiding this comment.
Pull request overview
This PR improves reliability of IndexCreationTool/WinGetSourceCreator by ensuring failures from invoked child processes (e.g., signtool, makeappx) propagate as exceptions instead of allowing a successful (0) exit after partial failure. It also updates E2E local source test data to avoid attempting to sign an installer type that should not be signed.
Changes:
- Update
WinGetSourceCreator’s sharedRunCommandhelper to throw when a child process exits non-zero. - Refactor
makeappx unpackto use the sharedRunCommandhelper (so failures are surfaced consistently). - Update E2E
localsource.jsonto setSkipSignature: truefor the font installer.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/WinGetSourceCreator/Helpers.cs |
Centralizes child process failure handling by throwing on non-zero exit codes (and refactors makeappx unpack to use the shared helper). |
src/AppInstallerCLIE2ETests/TestData/localsource.json |
Prevents signing for the font test installer via SkipSignature, aligning test data with the tool’s signing behavior. |
| if (p.ExitCode != 0) | ||
| { | ||
| throw new Exception($"Command failed with exit code {p.ExitCode}: {command} {args}"); | ||
| } |
There was a problem hiding this comment.
Maybe we could just include {command}.
In the pipelines this shouldn't be an issue, since Azure Pipelines automatically replaces any secret with ***, but still seems worth changing.
📖 Description
IndexCreationTool can run several child processes. If any of them fails, index creation has partially failed and I expect an exception, but IndexCreationTool exits 0. This PR throws an exception with the exit code and command/args to assist with debugging. I also updated
makeappx unpackto use the sharedRunCommandfunction.I suspect IndexCreationTool isn't a stable/supported dependency, but thought this was worth submitting anyway.
🔗 References
Example SignTool error: https://github.com/pl4nty/winget-extras/actions/runs/26671649037/job/78615909376
🔍 Validation
I don't have a VS toolchain on hand, so I'd love if someone can run the ADO tests on this PR. Then I'll validate in my CI from the built IndexCreationTool.
✅ Checklist
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow