fix(packager): return success status from doSign function calls #7431
Conversation
🦋 Changeset detectedLatest commit: 02f1e04 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for car-park-attendant-cleat-11576 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
nsrCodes
changed the title
There was a problem hiding this comment.
Thank you so much for calling out this issue!
I found that the BluebirdPromise.map doesn't require return as this.sign throws an error if it fails. We can just return true after signing completes.
I'm going to pick up this work and file a new PR. I'll take a larger stab at refactoring this code (and investigate some other Promise<any> that ended up letting this issue sneak by intellisense.
| @@ -269,6 +271,7 @@ export class WinPackager extends PlatformPackager<WindowsConfiguration> { | |||
| throw e | |||
| } | |||
| } | |||
| return didSign | |||
There was a problem hiding this comment.
This didSign flag logic needs to be flipped in order to handle the continue within the catch
let didSign = false
for (let i = 0; i < 3; i++) {
try {
await sign(options, this)
didSign = true
break
} catch (e: any) {
// https://github.com/electron-userland/electron-builder/issues/1414
const message = e.message
if (message != null && message.includes("Couldn't resolve host name")) {
log.warn({ error: message, attempt: i + 1 }, `cannot sign`)
continue
}
throw e
}
}
return didSign
| @@ -366,7 +369,9 @@ export class WinPackager extends PlatformPackager<WindowsConfiguration> { | |||
| if (this.shouldSignFile(file)) { | |||
| const parentDir = path.dirname(file) | |||
| if (parentDir !== packContext.appOutDir) { | |||
| return new CopyFileTransformer(file => this.sign(file)) | |||
| return new CopyFileTransformer(async file => { | |||
| await this.sign(file) | |||
There was a problem hiding this comment.
Is this change intentional?
file => this.sign(file) returns Promise<boolean>
async file => { await this.sign(file) } returns Promise<void> due to the brackets
There was a problem hiding this comment.
I was getting linting issues that this callback expected a return type of Promise<void>, which is why I made the change, but this seems like a linting issue which you fixed in the accompanying PR.
Removing these changes from here now
| @@ -450,7 +450,7 @@ export default class MacPackager extends PlatformPackager<MacConfiguration> { | |||
| await BluebirdPromise.map(readdir(packContext.appOutDir), async (file: string): Promise<any> => { | |||
There was a problem hiding this comment.
If you change the return of this larger function to Promise<boolean>, intellisense will still flag that this function isn't returning the correct values. I'll take a look on refactoring this function entirely
Recent changes in platform packager afterSign hook show that the correct success status was not being returned after signing the app on windows and macOs
nsrCodes
force-pushed
the
fix-packager-sign-status
branch
from
af9d68b to
db2db2c
Compare
nsrCodes
force-pushed
the
fix-packager-sign-status
branch
from
db2db2c to
b403b5e
Compare
|
@mmaietta I have rebased this with master which contains your larger rewrite. Doing this just for completeness, and a few small improvements I thought were relevant. Thanks for quickly releasing the fix! |
mmaietta
changed the title
Recent changes (#7311) in platform packager
afterSignhook show that the correct success status was not being returned after signing the app on macOSThis results in the app being signed but the terminal has the following message
found this on electron-builder v24.0.0-alpha.12
Couldn't get a local development environment properly setup, so hoping someone could help test if these help solve the issue