Skip to content
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

squirrel packages entire electron.exe in every update (electron-winstaller) (fix already in master) #1784

Closed
MichaelBelousov opened this issue Jan 24, 2022 · 6 comments · Fixed by #1809

Comments

@MichaelBelousov
Copy link

MichaelBelousov commented Jan 24, 2022

Squirrel version(s)
2.01-eef3746

Description
electron-winstaller's vendored squirrel's bsdiff fails to diff electron.exe's which differ only by 4 bytes, the 4 references to updated versions embedded in the exe. This causes squirrel to package the entire electron.exe every update.

As discussed in #1287, this is already fixed in master. But for some reason it was never cherry-picked to the development branch from which the electron-winstaller vendored binaries are from

looks like master has a fix here: afe47c9
but the version in electron winstaller is here: eef3746
according to git branch --contains eef37460ae that squirrel version (2.0.1) comes from the develop branch which does not have this fix

The PR to master where a fix was added #1421

Steps to recreate
See #1287

  1. use electron-forge/electron-packager/electron-winstaller to build a squirrel package for your electron app
  2. bump the patch version for your app
  3. use electron-forge/electron-packager/electron-winstaller to generate an update for your app
    this invokes their vendored 2.01 squirrel

Expected behavior
A small-ish patch for the app's renamed electron.exe executable is generated rather than the entire file placed in the update.

Actual behavior
squirrel's bsdiff fails because there is only is no bsdiff-extra-data generated by diffing this case, and there is a divide by zero error when trying to write an empty byte stream for the diff. The error is taken as reason to just stuff the binary file in the update if diffing failed.

@MichaelBelousov MichaelBelousov changed the title (electron-winstaller version of squirrel) squirrel packages entire electron.exe in every update (electron-winstaller version of squirrel) Jan 24, 2022
@MichaelBelousov MichaelBelousov changed the title squirrel packages entire electron.exe in every update (electron-winstaller version of squirrel) squirrel packages entire electron.exe in every update (electron-winstaller version of squirrel) (fix already in master!) Jan 24, 2022
@MichaelBelousov MichaelBelousov changed the title squirrel packages entire electron.exe in every update (electron-winstaller version of squirrel) (fix already in master!) squirrel packages entire electron.exe in every update (electron-winstaller) (fix already in master!) Jan 24, 2022
@MichaelBelousov
Copy link
Author

MichaelBelousov commented May 10, 2022

@anaisbetts (sorry for the ping, I believe you're still the de facto maintainer/permission-holder) if I make a PR cherry-picking this fix into the development branch and then talk to the electron-winstaller maintainers about releasing a new version picking up the change, will the PR get accepted?

@MichaelBelousov MichaelBelousov changed the title squirrel packages entire electron.exe in every update (electron-winstaller) (fix already in master!) squirrel packages entire electron.exe in every update (electron-winstaller) (fix already in master) Jun 1, 2022
@MichaelBelousov
Copy link
Author

MichaelBelousov commented Jun 1, 2022

@robmen is this something that you can accept via PR?

@robmen
Copy link
Contributor

robmen commented Jun 1, 2022

@MichaelBelousov so I'm clear, is this simply a commit (or two) in master that didn't get ported to develop? If so, I can cherry-pick them in my next round of changes (tonight).

@MichaelBelousov
Copy link
Author

MichaelBelousov commented Jun 1, 2022

yes exactly. The description above references the exact commit. I've been using a fork with the one commit cherry-picked.
For this to be picked up by electron consumers such as myself, a new version of electron-winstaller would have to be released as well consuming this. Do you know who can be asked from their side to do so?

@robmen
Copy link
Contributor

robmen commented Jun 2, 2022

Okay, I have the changes in master in develop adn everything should be straightened out there now. I'm evaluating my next step in the process automation.

However, your real question is what is the process for election-wininstaller to get an updated Squirrel with all these fixes. For that, I still need to lean on @anaisbetts as I haven't done that yet. Perhaps this should be the time I learn from @anaisbetts how but that means it'll happen pretty slowly. 😄

@MichaelBelousov
Copy link
Author

@robmen would it be appropriate for me to open a new tracking issue for updating the electron-winstaller package with the latest squirrel?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants