Update to electron-builder 22 (fixes SUID sandbox issues)

[pabzm]

electron-builder >= 22.0.0 fixes the setuid problem with the sandboxing helper: electron-userland/electron-builder#4163

This helper is required if kernel.unprivileged_userns_clone is not allowed (0), e.g. on Debian and derivates. (On other linux systems user name-spaces are used instead of the sandboxing helper.)

Fixing the sandbox-helper problem would enable us to update electron (see #895 and #948), which is apparently required to get into the MacOS app store (#1134, electron/electron#20027).

The update is jumping to a new major version, but the Changelog doesn't list any breaking changes, so I suppose this shouldn't make problems.

[Simon-Laux]

can some folks test the appimage of this PR? I forgot the password of my debian VM
https://download.delta.chat/desktop/preview/deltachat-desktop-1400.AppImage

[Simon-Laux]

Would be really awesome if this works.

[Simon-Laux]

Could you upgrade electron in this pr, too?
So that we can give the preview builds to debian folks to test it.

[pabzm]

Doesn't work... :( Oh my, it sounded like a good plan. I'll try to find out, what's wrong.

You don't need debian to test, just do sudo sysctl kernel.unprivileged_userns_clone=0.

[pabzm]

Hmm, maybe this runs only after installing a packages somewhere, but not during packaging? https://github.com/electron-userland/electron-builder/pull/4163/files

I'll investigate tomorrow.

[pabzm]

Well, duh... The commit message of electron-userland/electron-builder#4163 says that this is a fix for the debian package. So maybe this does help debian people by enabling them to use the debian-package at least? Could someone test that?

[pabzm]

Works! 🎉

I installed a locally built .deb in a Debian Buster (v10) VM and could start it, I'd say that's a sufficient check?

From my point of view we could go down this route. Debian users will still not be able to use the AppImage out of the box, but I think that's a sufferable loss.

[pabzm]

I also checked that running the flatpak-installed thing (from flathub.org, not locally modifed) works, so Debian users had one option left anyway. Now they'd have two.

[Simon-Laux]

linking some related links:
electron-userland/electron-builder#4495
electron-userland/electron-builder#4496

[Simon-Laux]

I agree to merge this, but I'm still a bit unsure about the AppImage...
Let's do it and see what happens - YOLO 😁 .

[Simon-Laux]

we should scroll up from this https://www.electronjs.org/docs/breaking-changes#planned-breaking-api-changes-40 and check for apis we use.

Update: (results of reading it)

https://www.electronjs.org/docs/breaking-changes#new-browserwindow-webpreferences- (changed defaults)
unlikely, but maybe from interest:
https://www.electronjs.org/docs/breaking-changes#winsetmenunull

[Jikstra]

Seems like we can upgrade electron & builder without bigger issues. For the AppImages we could still stay on electron 4 if that helps. I would still prefer to merge this during the next release cycle.

[pabzm]

@Simon-Laux: Why did you merge this despite open questions? @Jikstra had suggested to stay with electron 4 for the AppImage — I don't agree to that, but we should not ignore each another like this.

@Jikstra Wouldn't that mean to maintain different dependencies for different build targets? I don't know how this could work, and I am afraid of the complexity it brings, and I think it's not worth the effort: People shouldn't use AppImage anyway, if any other alternative works (AppImage has no validated download, no update-channel, etc.).
I'd rather suggest to put some effort into setting up a Debian repository in order to make it easy for Debian users to install Delta Chat.

[Simon-Laux]

I thought everything was solved? I also discussed this with jikstra IRL. Debian might have an error then on appimage start I don't see that as a big deal for now, we can upgrade electron builder again when electron-userland/electron-builder#4496 is merged to fix the appimage again.

(AppImage has no validated download, no update-channel, etc.).

We could fix that if we want, but I don't value it important enough right now. About the incompatibilities I checked again before merging and the changed default does not affect us, because we don't use a preload script. -- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

[pabzm]

Ok, thank you for the explanation. I didn't know you had been talking with jikstra AFK. Could you next time drop a hint, so I won't bother you again in such a case?

Also thanks for checking out electron's breaking changes!

[Simon-Laux]

I'll make sure to not forget to mention IRL/AFK decisions here, in the future. PS: Electron 7 is more breaking than 6, so good choice. Am 4. Februar 2020 14:48:43 MEZ schrieb Pablo <[email protected]>:

…

Ok, thank you for the explanation. I didn't know you had been talking with jikstra AFK. Could you next time drop a hint, so I won't bother you again in such a case? Also thanks for checking out electron's breaking changes! -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1400 (comment)

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.