-
Notifications
You must be signed in to change notification settings - Fork 39
fix: be lenient in parsing notarytool output #257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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.
Co-authored-by: Kevin Cui <github@bugs.cc>
|
Fixed the 2 CI failures, feel free to re-run @BlackHole1 |
|
Hmm: |
Signed-off-by: Kevin Cui <bh@bugs.cc>
BlackHole1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| try { | ||
| parsed = JSON.parse(jsonOut); | ||
| } catch (err) { | ||
| throw new Error(`Could not parse notarytool output: \n\n${rawOut}`); |
There was a problem hiding this comment.
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 🙇
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:
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.