fix(suite-desktop-core): fix async error in Windows signing#17446
fix(suite-desktop-core): fix async error in Windows signing#17446
Conversation
| const TOKEN_PASSWORD = process.env.WINDOWS_SIGN_TOKEN_PASSWORD; | ||
|
|
||
| await require('child_process').exec( | ||
| require('child_process').execSync( |
There was a problem hiding this comment.
in comment in the original PR, I thought this could be a problem, so I verified the electron-builder source code.
Still, it seems that the signing was finished async, after electron-builder calculated SHA hash and emitted latest.yml. The singing then changed the binary, which then produced inconsistent hash.
WalkthroughThe changes modify the implementation of the Windows signing function. The function's execution method has been updated to use a synchronous command execution approach provided by ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/scripts/sign-windows.ts (1)
5-5: Consider removing theasynckeyword.Since the function now uses synchronous execution and doesn't contain any
awaitstatements (hence the ESLint directive), you could consider removing theasynckeyword from the function signature while still returning a Promise to satisfy the TypeScript requirements.- const signWindows: CustomWindowsSign = async configuration => { + const signWindows: CustomWindowsSign = configuration => {However, verify with electron-builder documentation if the function must be declared as async or if returning a Promise is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/suite-desktop-core/scripts/sign-windows.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: e2e-test-suite-web (@group=wallet)
- GitHub Check: e2e-test-suite-web (@group=other)
- GitHub Check: e2e-test-suite-web (@group=metadata2)
- GitHub Check: e2e-test-suite-web (@group=metadata1)
- GitHub Check: e2e-test-suite-web (@group=settings)
- GitHub Check: e2e-test-suite-web (@group=device-management)
- GitHub Check: e2e-test-suite-web (@group=suite)
- GitHub Check: Linting and formatting
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (1)
packages/suite-desktop-core/scripts/sign-windows.ts (1)
3-4: Synchronous signing fixes the hash inconsistency issue.This change resolves the issue where asynchronous signing caused inconsistent SHA hashes. By switching to
execSync, we ensure the signing process completes before electron-builder continues with hash calculation.This matches the concern in the previous PR where the signing was finishing async, after electron-builder calculated SHA hash and emitted
latest.yml.Also applies to: 18-23
Description
In #17056 and #17234, the electron-builder hook for windows signing was rewritten from CJS to TS.
➡️ fix inconsistent SHA hashes caused by async code.
Screenshots:
Verified locally by first changing L19 to
Using a locally generated
.pfxfile with password 1234.Then I built and verified that SHA sums are the same (starts with
ajWzamhw)