fix: generate stubExecutableExe and sign it #8959
Conversation
|
packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts
Outdated
Show resolved
Hide resolved
|
|
|
Looks like |
|
Looks like it works sans the package.json not including vendor in the package - good job @beyondkmp I also checked that the number of signings remained the same |
There was a problem hiding this comment.
Adding the vendor directory adds a few megabytes (quick maffs) to the repo, which is the antipattern from what electron-builder-binaries is supposed to be used for. IIRC though, the squirrel windows target/package must be installed separately and isn't part of the electron-builder dependency tree, right? In this case, I think it's safe to add the vendor files (albeit I'll still need to verify file origin). In general though, we should avoid adding vendor files directly to electron-builder unless absolutely necessary (which is the case with this fix)
packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts
Outdated
Show resolved
Hide resolved
packages/electron-builder-squirrel-windows/src/SquirrelWindowsTarget.ts
Outdated
Show resolved
Hide resolved
Yup, |
my bad. Added. |
|
These vendor files are copied from the GitHub Actions workflow at https://github.com/beyondkmp/Squirrel.Windows/actions/runs/13871759240/job/38819263248 and the other files(like 7zip,nuget) are copiled from https://github.com/electron/windows-installer/tree/main/vendor. https://github.com/Squirrel/Squirrel.Windows/pull/1903/files |
|
@t3chguy |
|
@mmaietta Do you have any thoughts on this MR? |
|
So a couple notes:
In the current state, I'm not in favor of merging this PR. @beyondkmp, what if I wrote a script that |
|
@mmaietta I think it’s fine. I’ve already cloned it, and it can run successfully on GitHub Actions, and the build completes without issues. You can refer to this directly. |
|
@beyondkmp if we were to use Squirrel.Windows distributed via electron-builder-binaries (once I get a CI/CD set up there), do you think we'd need to revert the electron-winstaller changes back to the previous implementation then? |
|
It probably won't work well, as the newer version of Squirrel Windows has changed some things. My suggestion is still to use the Windows Installer. When there’s a new version, it can be synced directly. If we use our own implementation, some issues may not be synced in time. |
I'm against using a fork for the project. I'd rather If changes are needed in a dedicated |
|
What I mean is that you can refer to how this fork uses actions to automatically generate builds, rather than saying that you should use this fork directly. |
|
The purpose of electron-builder-binaries is to provide vendor files on-demand based on the use-case/packaging targets required for a build. I'm against committing any vendor files to this project directly. Willing to make exceptions though, just not for files that are measured in megabytes |
ok, got it. I'll set it to draft status first, and once the Squirrel.Windows update in electron-builder-binaries is complete, we can come back to update this PR. |
|
@beyondkmp new squirrel.windows in electron-builder-binaries with your patch applied can be found in this "compile" PR. (The CI/CD takes care of generating and committing the artifacts post-merge of the PR through Changesets GHA) |
|
squirrel.windows@1.0.0 using Squirrel 2.0.1 artifact has been released |
… binaries - Added logic to download and copy custom Squirrel binaries if the specified vendor directory is not accessible. - Removed outdated vendor binaries and configuration files to streamline the package.
…line package structure
…proved functionality
…e correct path handling
…ure proper installation
| const filePath = path.join(tmpVendorDirectory, file) | ||
| log.debug({ file: filePath }, "signing vendor executable") | ||
| await this.packager.sign(filePath) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor: Can avoid the for-if-break approach with a find
const file = files.find(f => f === "Squirrel.exe")
That being said, it looks like we're just trying to sign that one file, right?
const filePath = path.join(tmpVendorDirectory, "Squirrel.exe")
if (filePath) { // do we need this check or should Squirrel.exe always be present?
log.debug({ file: filePath }, "signing vendor executable")
await this.packager.sign(filePath)
}
| await fs.promises.cp(vendorDirectory, tmpVendorDirectory, { recursive: true }) | ||
| if (customSquirrelBin && customSquirrelBin.length > 0) { |
There was a problem hiding this comment.
I'm not sure what this is checking for. Are you intending?
!isEmptyOrSpaces(customSquirrelBin) && await exists(customSquirrelBin)
| await this.packager.sign(stubExePath) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Wait, why do we need to copy a ${fileNameWithoutExt}_ExecutionStub.exe for every .exe file in the appOutDir? Is that to be expected or are there only specific files that we need the stub for?
fix #8952
Root Cause
when createExecutableStubForExe is executed, WriteZipToSetup writes information to the file, essentially creating a new file, which invalidates the original signature.
https://github.com/Squirrel/Squirrel.Windows/blob/51f5e2cb01add79280a53d51e8d0cfa20f8c9f9f/src/Update/Program.cs#L633-L647
How to fix
Apply a patch to the Squirrel Windows source code(Squirrel/Squirrel.Windows#1903). For the existing stub exe files, don't generate them anymore. Then, a new stub exe can be generated in Electron Builder and signed.