-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
>26.0.1 gets stuck on Windows signing the squirrel exe #8854
Comments
On a retry with debug logging enabled it seems to hang directly after signing the MSI https://github.com/element-hq/element-desktop/actions/runs/13200748516/job/36851975119 |
electron-builder run with debugging and crash from electron-windows-sign due to it not using e-b's the reason I believe |
I've had to roll back to get code signing working again which is a shame as it means I lost ARM64 support. I will set up a better environment to test including code signing before the next e-b upgrade -= element-hq/element-desktop#2131 should be that |
@t3chguy Do you use a custom sign function? |
if CSC_LINK is used, electron-builder does have unit tests (UT) to ensure that signing can pass.
|
No, we pass signtool options of subject Name and sha1. We also specify the signtool executable via environment as we need a modern one which is compatible with the eSignerCKA |
This issue can be reproduced in the GitHub Action. However, signing works fine locally on Windows 11, and the problem cannot be reproduced there. The machines in GitHub Actions run on Windows Server 2022. It's still unclear why it fails in GitHub Actions, and it will take some time to investigate the cause. |
The fact that the vendored signtool is being executed rather than the one specified in |
However, when I tested it locally, I was able to use the |
@beyondkmp if you look at https://github.com/element-hq/element-desktop/actions/runs/13204321507/job/36863804553#step:17:599
You can see a successful electron-builder sign using
It looks like for squirrel signing this function is not entered as there's no |
Based on the source code of the Windows installer, if the In my local tests, it succeeded because everything was on the same C drive. However, I noticed that on GitHub Actions, the data and |
@beyondkmp where are you seeing the fake sign tool? I see it calling an earlier code path if |
This looks to be similar if not the same as electron/windows-installer#404 & more concretely electron/windows-installer#508 Definitely seems like the bug is upstream of e-b but it is a monumental bug to be shipping either way. I'm going to look into a patch-package fix workaround in the coming week, as I do want the other benefits of the major upgrade. |
@t3chguy electron/windows-sign#12 (comment) It seems that |
@beyondkmp what's the plan then? to switch to |
I personally we should fix upstream, as I'm getting more involved with working with the electron Is there a minimum repro repo for this or do I need to use the full project you're using, the latter will prob be more difficult to set up on my arm64 Windows VM, so just wondering if we can consolidate the work to a minimum repro. Seems like your signing env might be something we'll need to repro locally with though? |
I have eSignerCKA working with demo credentials so it should be easier to spin up a minimum repro, but I'm currently sick and have severe brain fog so don't think I'll be able to get you a minimum repro.
Hope that helps @mmaietta |
@t3chguy I'll try to test that out locally. Best wishes on getting better and feeling healthy soon! |
@mmaietta oh almost forgot, we tell electron-builder to use the system signtool explicitly https://github.com/element-hq/element-desktop/blob/t3chguy/test-windows-signing/.github/workflows/build_windows.yaml#L39 as eSignerCKA needs a fairly modern signtool |
Weird, I get |
Wait, I don't understand this. Looking at the code, it should just execute our hook, no additional logic |
The Inside |
@t3chguy can you try out this
|
@mmaietta https://github.com/element-hq/element-desktop/actions/runs/13290420431/job/37109610542?pr=2132#step:17:1456
|
Though it did seemingly pass on the x64 build https://github.com/element-hq/element-desktop/actions/runs/13290932109/job/37111358677?pr=2132 - so the other error may be yet another major issue 🙈 I loaded up the x64 zip from github actions into a Windows box and: Looks like the Squirrel Setup file is missing signature vs one made using older e-b Also looks like the icon of the Setup file is incorrect, and the name is just Concerningly, I also see a bunch of things getting signed which are not part of the package but rather look like build/packaging utilities: eSigner costs per-signature so signing additional files would increase expenditure significantly given we do at least 1 build a day across 2 architectures and want to add arm64. |
Oomph, yeah, we definitely can't cause that impact on signing costs. Thanks for calling it out! CC @beyondkmp looks like we need an alternative way to handle this signing logic and/or open a PR upstream for allowing more custom logic flows (e.g. Azure signing) that can't leverage |
@mmaietta electron/windows-installer#548 However, based on my investigation, under the current architecture, implementing hookFunction in the upstream is not feasible. Moreover, Electron Forge also only supports hookModulePath. |
@beyondkmp & @mmaietta thank you for persevering with this, great work. |
Before update (26.0.1) https://github.com/element-hq/element-desktop/actions/runs/13175306673/job/36773250686
After update to 26.0.6 https://github.com/element-hq/element-desktop/actions/runs/13196581336/job/36839196683 it errors when calling upon a vendored signtool.exe from electron-winstaller/electron-windows-sign - the signing we use via eSignerCKA requires a rather modern signtool so we prefer the system one by use of the
SIGNTOOL_PATH
envvar which electron-builder respects.electron-win-sign
however does not respect it, it instead readsWINDOWS_SIGNTOOL_PATH
upon changing to which signing just seems to hang. https://github.com/element-hq/element-desktop/actions/runs/13197854827/job/36843095558I'm surprised to see
D:\a\element-desktop\element-desktop\node_modules\electron-winstaller\vendor\signtool.exe
being called given e-b is passing a signing hookFunction so should be using its own signtool handling -electron-builder/packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts
Lines 144 to 150 in f4fc04a
Maybe that if-statement isn't being entered due to the condition not doing the same thing as the old signing code - looks like other areas in the code just look at
options.sign
potentially https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/platformPackager.ts#L370The text was updated successfully, but these errors were encountered: