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

[query] [electron-updater] Why is a subset of linux distros supported for autoupdates? #8370

Open
xyloflake opened this issue Jul 23, 2024 · 29 comments

Comments

@xyloflake
Copy link
Contributor

So as the title states, I was looking for a solution to make auto-update work on platforms like arch linux. I read a comment made by @develar that it is by intention - that some platforms are not supported. However I failed to find any issue that mentioned the "why". I'd like to know why it is unsupported.

Thanks in advance!

@xyloflake
Copy link
Contributor Author

What are the problems associated with implementing it for some distros is another thing I'd like to know :)

@mmaietta
Copy link
Collaborator

I'm not familiar as to why more aren't supported. When I joined the crew, originally it was only AppImage. I implemented deb and rpm autoupdates after a significant testing period. For other platforms like arch linux, someone will need to do the implementation (happy to provide guidance) and test the different package managers that could be utilized.

Basically, for implementation, all that is needed are the spawn cmds for the other platforms to be supported. Here's how deb updates were implemented

protected doInstall(options: InstallOptions): boolean {
const sudo = this.wrapSudo()
// pkexec doesn't want the command to be wrapped in " quotes
const wrapper = /pkexec/i.test(sudo) ? "" : `"`
const cmd = ["dpkg", "-i", options.installerPath, "||", "apt-get", "install", "-f", "-y"]
this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`])
if (options.isForceRunAfter) {
this.app.relaunch()
}
return true
}

@xyloflake
Copy link
Contributor Author

xyloflake commented Jul 24, 2024

I'm not familiar as to why more aren't supported. When I joined the crew, originally it was only AppImage. I implemented deb and rpm autoupdates after a significant testing period. For other platforms like arch linux, someone will need to do the implementation (happy to provide guidance) and test the different package managers that could be utilized.

Basically, for implementation, all that is needed are the spawn cmds for the other platforms to be supported. Here's how deb updates were implemented

protected doInstall(options: InstallOptions): boolean {
const sudo = this.wrapSudo()
// pkexec doesn't want the command to be wrapped in " quotes
const wrapper = /pkexec/i.test(sudo) ? "" : `"`
const cmd = ["dpkg", "-i", options.installerPath, "||", "apt-get", "install", "-f", "-y"]
this.spawnSyncLog(sudo, [`${wrapper}/bin/bash`, "-c", `'${cmd.join(" ")}'${wrapper}`])
if (options.isForceRunAfter) {
this.app.relaunch()
}
return true
}

I would be super happy to implement for arch linux, am I allowed to do that? I thought of making a PR but was afraid of it being rejected because of what @develar said.

Even before the above instructions were posted, I had figured it out on how to implement it :)

I also have another patch for a repo I work on. It's for electron-builder 24.13.3 since that's the stable version, and it reduces the size by around 7MB! For larger codebases, this could result in an even more compressed installer, which is awesome. However, I've found some anomalies in the codebase that are bugging me before I make the PR. Could you check them out?

Thanks a ton! Can't wait to hear your thoughts! 🚀

@mmaietta
Copy link
Collaborator

mmaietta commented Jul 24, 2024

I'm not opposed to supporting arch linux if you're willing to put up a PR! We'll just need to flag it as a Beta Feature like the deb/rpm autoupdaters.
I'm not sure what you're referring to that develar would reject. Would you mind providing a link to that conversation?

I'm not sure I follow w.r.t. the anomalies. The v25 CI builds are passing correctly.

Re: the node-gyp errors, I'm happy to take a look if you can put together a minimum reproducible repo? (Or do I just use the muffon repo itself with npm run package:all?)

@xyloflake
Copy link
Contributor Author

xyloflake commented Jul 25, 2024

I'm not opposed to supporting arch linux if you're willing to put up a PR! We'll just need to flag it as a Beta Feature like the deb/rpm autoupdaters.

Super cool! Will make a PR after patching 24.13.3 in the muffon repo and will test out in the betas if the autoupdates work well or not for arch! Sounds alright?

I'm not sure what you're referring to that develar would reject. Would you mind providing a link to that conversation?

Yes! Sure. Here is the issue in which develar states "Some platforms are intentionally not supported" - #3844

Just like you stated earlier; at that time only appImage was supported.

I'm not sure I follow w.r.t. the anomalies. The v25 CI builds are passing correctly.

Ah, the build info is outdated as of now. What I was trying to reference is

Electron builder internally resets the compression configuration option to normal for Windows, albeit without explicit rationale. Consequently, this patch intervenes to prevent the reset to normal exclusively for Windows systems, as the aforementioned algorithm cannot operate optimally on other platforms.

As in why it's reset to normal without any reason? It works fine without that... Also, only Windows is affected by this "normal" thing iirc.

Re: the node-gyp errors, I'm happy to take a look if you can put together a minimum reproducible repo? (Or do I just use the muffon repo itself with npm run package:all?)

Oh forget that please it may be outdated as of now because I see the CI/CD pipeline getting through fine. At the time of writing that, the CI/CD pipeline also failed 😄.

muffon does not have the error as we don't use electron-builder v25/v24 alphas for stability purposes. I'll check over the weekend if the issue persists or not and will notify based upon that!

Thank you so much for the support! Is there any community chat server for electron-builder? I'd suggest something like Slack/Discord/Matrix/etc. for chatting about and casually asking about stuff 😅

@mmaietta
Copy link
Collaborator

Very interesting comment regarding using package managers. I could understand the use case there; I guess I interpreted that electron apps were designed to allow distribution outside of package managers or App Stores.

Related, I discovered that there's an electron-builder channel within the Electron discord community https://discord.com/channels/745037351163527189/746375188358365204

@xyloflake
Copy link
Contributor Author

Very interesting comment regarding using package managers. I could understand the use case there; I guess I interpreted that electron apps were designed to allow distribution outside of package managers or App Stores.

Were designed or were not designed? I feel like you wanted to say NOT designed?

Related, I discovered that there's an electron-builder channel within the Electron discord community https://discord.com/channels/745037351163527189/746375188358365204

Cool, but I think this deserves a dedicated discord server since doing all of development and asking questions might be a pain in one single channel.

Can I also have your discord and reach out so I can ask some more questions if you don't mind?

Thank you so much for the reply! Can you uhh maybe address the compression regarding concerns as well?

@mmaietta
Copy link
Collaborator

I interpreted that electron apps were designed to allow distribution outside of package managers or App Stores.

I say "were designed" because that's why I think auto-update support should be allowed for other linux distros. Under-the-hood, the auto-updaters are just calling the package managers anyhow.

--

That's a fair reason. I used to have the Github Discussions page but wasn't receiving enough notifications or had the hours to be able to monitor it. I removed it in favor of just having support tickets as Github Issues for traceability.

--

Re:

As in why it's reset to normal without any reason? It works fine without that... Also, only Windows is affected by this "normal" thing iirc.

I haven't the slightest clue as to why that logic exists. The commit is over 7 years old and was only part of a refactor, so the core reason is obfuscated to me. My best guess is that maximum compression causes the update differential to vary more widely, causing (almost) the entire app to be downloaded on each update instead of optimized for differential downloading.

@xyloflake
Copy link
Contributor Author

I interpreted that electron apps were designed to allow distribution outside of package managers or App Stores.

I say "were designed" because that's why I think auto-update support should be allowed for other linux distros. Under-the-hood, the auto-updaters are just calling the package managers anyhow.

Okay, I got what you mean now! Yup, that definitely makes sense. So I'll be pushing over a PR most probably over weekend. Might be a bit late since I've got school and stuff...

That's a fair reason. I used to have the Github Discussions page but wasn't receiving enough notifications or had the hours to be able to monitor it. I removed it in favor of just having support tickets as Github Issues for traceability.

That's a very good reason imho. However discord also has a forum feature that makes having supports tickets like GitHub issues possible right from discord! Also, I meant to say that there should be a discord server for specifically contributing to electron-builder monorepo as it may need answers to quite a lot of questions people might have while contributing; just like the legacy stuff about the compression we are referring to.

Re:

As in why it's reset to normal without any reason? It works fine without that... Also, only Windows is affected by this "normal" thing iirc.

I haven't the slightest clue as to why that logic exists. The commit is over 7 years old and was only part of a refactor, so the core reason is obfuscated to me. My best guess is that maximum compression causes the update differential to vary more widely, causing (almost) the entire app to be downloaded on each update instead of optimized for differential downloading.

I think that the specific decision of "max compression but entire app is downloaded on autoupdates" should be passed on to the end developer(?). Since this is all legacy stuff, can we run some tests and get a rough idea of if that can be removed or not? If that can be removed, we can get some actual compression on the installers.

Explicit optimization benchmark:
On muffon, before the custom compression type "ultra" was introduced, we used "store". This got us faster builds and no significant size difference between "store" and "maximum". The installers were about 91.3MB in size. After the implementation of "ultra": we now have 64MB installer binaries.

@xyloflake
Copy link
Contributor Author

My best guess is that maximum compression causes the update differential to vary more widely, causing (almost) the entire app to be downloaded on each update instead of optimized for differential downloading.

I think we should run some real world testing before coming to that conclusion? I don't think the thing about differential builds is correct... The entire app is downloaded anyways (?).

@xyloflake
Copy link
Contributor Author

@mmaietta how do I test stuff for electron-updater? Do I have to rebuild on CI for every code change?

@xyloflake
Copy link
Contributor Author

xyloflake commented Jul 27, 2024

@mmaietta I really tried hard to patch it but I'm having a VERY hard time testing the updates locally. Do you have any tips for testing auto updates locally? I literally have to package the application again and again for literally testing 1 line of code change and then wait for 12 minutes for build and finally figure out that I can't test the auto updates because I get checkForUpdatesAndNotify called, downloadPromise is null when I run the application.

Edit: I found out by reading the documentation -

Note that in order to develop/test UI/UX of updating without packaging the application you need to have a file named dev-app-update.yml in the root of your project, which matches your publish setting from electron-builder config (but in yaml format). But it is not recommended, better to test auto-update for installed application (especially on Windows). Minio is recommended as a local server for testing updates.

@xyloflake
Copy link
Contributor Author

Also, one more question, how do you determine if the deb, rpm or pacman is to be used?

@xyloflake
Copy link
Contributor Author

@mmaietta you haven't been replying since days. Are you okay?

@mmaietta
Copy link
Collaborator

Been on vacation 🙂

Since this is all legacy stuff, can we run some tests and get a rough idea of if that can be removed or not? If that can be removed, we can get some actual compression on the installers.
I think we should run some real world testing before coming to that conclusion? I don't think the thing about differential builds is correct... The entire app is downloaded anyways (?).

If you'd like to run some tests, please feel free to do so. The entire app is not supposed to be getting downloaded anyways unless the diff is genuinely that significant. There's been optimizations in the newer versions of electron-builder that optimizes the asar file ordering to improve differential updates as well.

how do I test stuff for electron-updater? Do I have to rebuild on CI for every code change?

This is the zsh alias set up on my mac for moving electron-builder/updater changes directly into a local project to avoid the CI/CD flow

alias resync="rsync -upaRv --include='*.js' --include='*.d.ts' --include='*.nsi' --include='*.json' --include='*/' --include='*.py*' --include='*.tiff' --exclude='*'  ~/Development/electron-builder/packages/./* node_modules/"

Call cmd resync from the project directory that is at the same level as electron-builder repo folder.

I literally have to package the application again and again for literally testing 1 line of code change and then wait for 12 minutes for build

For electron-updater code, this is the only route unfortunately. There's no way to test it via unit test due to the need for sudo.

Also, one more question, how do you determine if the deb, rpm or pacman is to be used?

This needs an updater added here:

switch (fileType) {
case "deb":
_autoUpdater = new (require("./DebUpdater").DebUpdater)()
break
case "rpm":
_autoUpdater = new (require("./RpmUpdater").RpmUpdater)()
break
default:
break
}

And your updater target added here:

private supportsAutoUpdate(target: string) {
return ["deb", "rpm"].includes(target)
}

@xyloflake
Copy link
Contributor Author

Been on vacation 🙂

Welcome back! Hope you had fun.

Update: you were 2 days late lol. I've got autoupdates working on pacman and found a bug which I've also fixed. muffon is currently undergoing beta testing for autoupdates on Pacman and as soon as it's declared stable, most probably in 18 hours or so from the time of writing this, I'll make a PR. Everything is ready just not pushing it to be sure that it indeed would work.

Since this is all legacy stuff, can we run some tests and get a rough idea of if that can be removed or not? If that can be removed, we can get some actual compression on the installers.
I think we should run some real world testing before coming to that conclusion? I don't think the thing about differential builds is correct... The entire app is downloaded anyways (?).

If you'd like to run some tests, please feel free to do so. The entire app is not supposed to be getting downloaded anyways unless the diff is genuinely that significant. There's been optimizations in the newer versions of electron-builder that optimizes the asar file ordering to improve differential updates as well.

Cool! That sounds incredible. I'd still want to run some tests regarding the efficiency of differential updates. Can you maybe point me to the code where the differential update logic is declared?

how do I test stuff for electron-updater? Do I have to rebuild on CI for every code change?

This is the zsh alias set up on my mac for moving electron-builder/updater changes directly into a local project to avoid the CI/CD flow

alias resync="rsync -upaRv --include='*.js' --include='*.d.ts' --include='*.nsi' --include='*.json' --include='*/' --include='*.py*' --include='*.tiff' --exclude='*'  ~/Development/electron-builder/packages/./* node_modules/"

Call cmd resync from the project directory that is at the same level as electron-builder repo folder.

Turned out you were late lol. I actually started an http server and changed the autoupdate server to generic so I could test it that way, an it worked superbly. I had no problems while testing that way.

I literally have to package the application again and again for literally testing 1 line of code change and then wait for 12 minutes for build

For electron-updater code, this is the only route unfortunately. There's no way to test it via unit test due to the need for sudo.

Yea, figured that out. Thanks.

Also, one more question, how do you determine if the deb, rpm or pacman is to be used?

This needs an updater added here:

switch (fileType) {
case "deb":
_autoUpdater = new (require("./DebUpdater").DebUpdater)()
break
case "rpm":
_autoUpdater = new (require("./RpmUpdater").RpmUpdater)()
break
default:
break
}

And your updater target added here:

private supportsAutoUpdate(target: string) {
return ["deb", "rpm"].includes(target)
}

Again, I've already made the code changes and implemented the autoupdates for Pacman and it works fine as per our beta testers.

I found out that app-builder-lib adds a file called package-type through which you can detect what package was used for the installation.

Thank you for your help! PR for Pacman incoming ASAP.

@xyloflake
Copy link
Contributor Author

Quick Demo for the time being. May not be able to make a PR today as I have the mentioned bug to fix.

staniel359/muffon#189 (comment)

@xyloflake
Copy link
Contributor Author

xyloflake commented Jul 31, 2024

@mmaietta why does the app freeze when electron-updater executes the sudo commands? You can see that in the vid above

@xyloflake
Copy link
Contributor Author

I think that's because spawnSync is used, which blocks the entire thread.

@mmaietta
Copy link
Collaborator

mmaietta commented Aug 1, 2024

Only spawnSync can be used in order for the app to not quit early and for "restart after update" functionality to be preserved. There's no way to make it async as no progress "listener" is available from the package manager(s)

@xyloflake
Copy link
Contributor Author

Only spawnSync can be used in order for the app to not quit early and for "restart after update" functionality to be preserved. There's no way to make it async as no progress "listener" is available from the package manager(s)

If I manage to make it async, can I make a PR?

@mmaietta
Copy link
Collaborator

mmaietta commented Aug 2, 2024

That sounds lovely!

@xyloflake
Copy link
Contributor Author

That sounds lovely!

Cool! Gotta work on it then!

@mmaietta
Copy link
Collaborator

mmaietta commented Aug 3, 2024

Awesome, thank you!

Also, sorry, just saw this question in one of your previous posts:

Cool! That sounds incredible. I'd still want to run some tests regarding the efficiency of differential updates. Can you maybe point me to the code where the differential update logic is declared?

I think you can look at DifferentialDownloader.ts for the differential logic, but the logic is obfuscated amongst multiple files AFAICT in https://github.com/electron-userland/electron-builder/tree/master/packages/electron-updater/src/differentialDownloader

@xyloflake
Copy link
Contributor Author

Awesome, thank you!

Also, sorry, just saw this question in one of your previous posts:

Cool! That sounds incredible. I'd still want to run some tests regarding the efficiency of differential updates. Can you maybe point me to the code where the differential update logic is declared?

I think you can look at DifferentialDownloader.ts for the differential logic, but the logic is obfuscated amongst multiple files AFAICT in https://github.com/electron-userland/electron-builder/tree/master/packages/electron-updater/src/differentialDownloader

Hi! Thank you for the reply~~

At the time of writing, I am preparing to make a PR for the pacman autoupdate support! Very excited for my first contribution here.

I would like to inform that I indeed did find a way to make quitAndInstall asynchronous. Even managed to preserve relaunch.

onQuit(handler: (exitCode: number) => void): void {
this.app.once("quit", (_: Electron.Event, exitCode: number) => handler(exitCode))
}

I changed this to use the before-quit event instead of quit and used event.preventDefault() to hault the quit.

  onQuit(handler: (exitCode: number) => void): void {
    this.app.once("before-quit", async (event: Electron.Event) => {
      handler(exitCode))
    }
  }

However, there is one downside (not really) - you don't get the exitcode. Why I say "not really" is because, it's not mandatory since the autoupdater will run before the app quits and it doesn't have to worry about how the app quits.

So I removed these lines:

if (exitCode !== 0) {
this._logger.info(`Update will be not installed on quit because application is quitting with exit code ${exitCode}`)
return
}

And converted the other functions to async as well. It works flawlessly. Is there any reason to "why" before-quit was not used?

@mmaietta
Copy link
Collaborator

mmaietta commented Aug 6, 2024

Well done!! Re:

Is there any reason to "why" before-quit was not used?

I don't recall there being any particular reason as to why. But it may be worth testing out the new async logic on deb/rpm builds as well just in case the before-quit does not work properly. I think it should be a safe change though

@xyloflake
Copy link
Contributor Author

xyloflake commented Aug 6, 2024

Well done!! Re:

Is there any reason to "why" before-quit was not used?

I don't recall there being any particular reason as to why. But it may be worth testing out the new async logic on deb/rpm builds as well just in case the before-quit does not work properly. I think it should be a safe change though

Thank you so much!

Then I'm all set to make a PR. As always, will use our beta testers to confirm that it's stable at first 😄

@xyloflake
Copy link
Contributor Author

@mmaietta new question: why has the design of the electron.build docs website not changed since a very long time? The website has no dark mode, looks ugly when you take modern design patterns in consideration, looks outdated, has a very weird color scheme and it's hard to tell properties apart.

Why was the styling never changed and are we allowed to change it now since develar's no longer active in this repo?

@xyloflake
Copy link
Contributor Author

Also @mmaietta, I really want to be a collaborator on this repo, what are the requirements/needs for it? Or is it just a matter of time and contributions?

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

No branches or pull requests

2 participants