Skip to content

Conversation

@msanft
Copy link

@msanft msanft commented Nov 5, 2025

We were seeing failures in our CI on notarytool
invocations, as warnings emitted by notarytool
were tried to be parsed as JSON.
An example for such a warning is the following:

✖ Finalizing package [FAILED: Failed to notarize via notarytool.  Failed with unexpected result:

warning: unhandled Platform key FamilyDisplayName
{"id":"4803438f-a484-493a-acfd-1ef504a1e70b","message":"Processing complete","status":"Accepted"}]

This makes it so the parsing can work with such
warnings, by finding the valid JSON line in the
output and only using that to parse from.

The less-hacky way to fix this would be to properly
differentiate between stdout and stderr in spawn.ts,
but I'm unable to test other commands and didn't
want to break stuff, so I address the issue for
notarytool only here.

Furthermore, this makes the assumption that Apple's
signing servers will only reply with a single JSON line.
This was the case for all my testing queries, but I'm
not sure if this holds true universally.

Still, I think this is much better than the current situation,
which just fails on whatever warning is emitted by notarytool.

We were seeing failures in our CI on notarytool
invocations, as warnings emitted by notarytool
were tried to be parsed as JSON.
An example for such a warning is the following:

```
✖ Finalizing package [FAILED: Failed to notarize via notarytool.  Failed with unexpected result:

warning: unhandled Platform key FamilyDisplayName
{"id":"4803438f-a484-493a-acfd-1ef504a1e70b","message":"Processing complete","status":"Accepted"}]
```

This makes it so the parsing can work with such
warnings, by finding the valid JSON line in the
output and only using that to parse from.

The less-hacky way to fix this would be to properly
differentiate between stdout and stderr in `spawn.ts`,
but I'm unable to test other commands and didn't
want to break stuff, so I address the issue for
notarytool only here.

Furthermore, this makes the assumption that Apple's
signing servers will only reply with a single JSON
line. This was the case for all my testing queries,
but I'm not sure if this holds true universally.

Still, I think this is much better than the current
situation, which just fails on whatever warning is
emitted by notarytool.
@msanft msanft requested a review from a team as a code owner November 5, 2025 14:04
@BlackHole1 BlackHole1 requested a review from a team November 6, 2025 04:15
Co-authored-by: Kevin Cui <github@bugs.cc>
@msanft msanft requested a review from BlackHole1 November 6, 2025 20:33
@msanft msanft changed the title notarytool: be lenient in parsing command output fix: be lenient in parsing notarytool output Nov 8, 2025
@msanft
Copy link
Author

msanft commented Nov 8, 2025

Fixed the 2 CI failures, feel free to re-run @BlackHole1

@msanft
Copy link
Author

msanft commented Nov 12, 2025

Hmm:

[11:13] msanft:notarize @ msanft/notarytool-json-fix-build is 📦 semantic 
❯ npm run lint

> @electron/notarize@0.0.0-development lint
> prettier --check "src/**/*.ts"

Checking formatting...
All matched files use Prettier code style!
[11:14] msanft:notarize @ msanft/notarytool-json-fix-build is 📦 semantic 
✦ ❯ git push --force-with-lease
Everything up-to-date

Signed-off-by: Kevin Cui <bh@bugs.cc>
@msanft msanft requested a review from BlackHole1 November 12, 2025 10:53
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM

@BlackHole1 BlackHole1 requested a review from a team November 12, 2025 17:09
try {
parsed = JSON.parse(jsonOut);
} catch (err) {
throw new Error(`Could not parse notarytool output: \n\n${rawOut}`);
Copy link
Member

Choose a reason for hiding this comment

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

Could we as a fallback try parse the entire output as JSON so that anything that worked before due to strange chunking or whatever keeps working.

Super appreciate the test coverage 🙇

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.

3 participants