feat(snap): add Snap compression option

[om26er]

Fixes #5881

Signed-off-by: Omer Akram [email protected]

[changeset-bot]

🦋 Changeset detected

Latest commit: 77c00af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
app-builder-lib	Major
builder-util	Major
dmg-builder	Major
electron-builder-squirrel-windows	Major
electron-builder	Major
electron-forge-maker-appimage	Major
electron-forge-maker-nsis-web	Major
electron-forge-maker-nsis	Major
electron-forge-maker-snap	Major
electron-publish	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[om26er]

Did more investigation and found the above change won't work. electron builder uses app-builder to create the snap archive, which currently has xz compression hard-coded. https://github.com/develar/app-builder/blob/4dbd42fea73c5d6526009de9d077375c46c0d41a/pkg/package-format/snap/snap.go#L288

Will first work with app-builder upstream to optionally support LZO compression for snap

[om26er]

Just a heads up, the fix for app-builder is now up develar/app-builder#65 -- Will likely need to adapt this PR once app-builder change lands (or may even have to wait for a new release of it ?)

[develar]

Thanks! I published [email protected]. The reason for breaking change — Go 1.17 requires macOS 10.13 High Sierra or later. I decided that it is ok.

[om26er]

Sorry for the delay, I will get back to this PR during the weekend.

[om26er]

Now hit by electron-userland/electron-builder-binaries#45

[ppd]

@om26er Is this still waiting for an app-builder release?

[ppd]

This PR works for me with app-builder-bin 4.1.0 (latest) and the missing hookup of the compression option:

diff --git a/packages/app-builder-lib/src/targets/snap.ts b/packages/app-builder-lib/src/targets/snap.ts
index 7f1dbd72..e6e0dbc9 100644
--- a/packages/app-builder-lib/src/targets/snap.ts
+++ b/packages/app-builder-lib/src/targets/snap.ts
@@ -222,6 +222,10 @@ export default class SnapTarget extends Target {
     if (this.isUseTemplateApp) {
       args.push("--template-url", `electron4:${snapArch}`)
     }
+
+    if (options.compression != null) {
+      args.push("--compression", options.compression)
+    }
     await executeAppBuilder(args)
 
     await packager.info.callArtifactBuildCompleted({
diff --git a/packages/builder-util/package.json b/packages/builder-util/package.json
index 178309cc..8e2166fa 100644
--- a/packages/builder-util/package.json
+++ b/packages/builder-util/package.json
@@ -18,7 +18,7 @@
     "7zip-bin": "~5.1.1",
     "@types/debug": "^4.1.6",
     "@types/fs-extra": "^9.0.11",
-    "app-builder-bin": "3.7.1",
+    "app-builder-bin": "4.1.0",
     "bluebird-lst": "^1.0.9",
     "builder-util-runtime": "workspace:*",
     "chalk": "^4.1.1",
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
index cf64bfb2..d151bb3b 100644
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -171,7 +171,7 @@ importers:
       '@types/js-yaml': 4.0.2
       '@types/source-map-support': 0.5.4
       7zip-bin: ~5.1.1
-      app-builder-bin: 3.7.1
+      app-builder-bin: 4.1.0
       bluebird-lst: ^1.0.9
       builder-util-runtime: workspace:*
       chalk: ^4.1.1
@@ -187,7 +187,7 @@ importers:
       '@types/debug': 4.1.7
       '@types/fs-extra': 9.0.12
       7zip-bin: 5.1.1
-      app-builder-bin: 3.7.1
+      app-builder-bin: 4.1.0
       bluebird-lst: 1.0.9
       builder-util-runtime: link:../builder-util-runtime
       chalk: 4.1.2
@@ -2899,8 +2899,8 @@ packages:
       normalize-path: 3.0.0
       picomatch: 2.3.0
 
-  /app-builder-bin/3.7.1:
-    resolution: {integrity: sha512-ql93vEUq6WsstGXD+SBLSIQw6SNnhbDEM0swzgugytMxLp3rT24Ag/jcC80ZHxiPRTdew1niuR7P3/FCrDqIjw==}
+  /app-builder-bin/4.1.0:
+    resolution: {integrity: sha512-rbdMe0sIVE95cpYqMQh4IFqhTDdB8LkKlTRcbO/Y3QleRYoIePejIbX774IYomYYzZbJfJuX7pLRiGvSdIxIYA==}
     dev: false
 
   /archiver-utils/2.1.0:

[ppd]

The default should probably be lzo. There is mostly just one reason for xz: file size. The abysmal decompression speed is absolutely terrible for the kind of applications that are usually built with Electron.

The kde-neon extension wants to enable lzo by default in the future: canonical/snapcraft#3595

[om26er]

@ppd can you propose the change to my PR, I'll merge that in

[ppd]

@om26er om26er#1
Contains the changes from the patch above and the lzo default.

[ppd]

@om26er there are some additional changes @ om26er#2

Actually default to lzo by including it in the snapcraft template.
Also construct the builder argument from the descriptor & not from the option. We would ignore the compression option from the default template otherwise.

Also included: two rather superficial tests. I think the jest snapshots need to be updated, still.

[ppd]

@develar I don't know what to do about this:

/root/project/node_modules/.pnpm/[email protected]/node_modules/app-builder-bin/linux/x64/app-builder exited with code ERR_ELECTRON_BUILDER_CANNOT_EXECUTE

The tests run without a problem locally.

@om26er Also, the jest snapshots need to be regenerated: om26er#3

[ppd]

@om26er A changeset is needed, I guess. What do you think?

Something like

---
"app-builder-lib": minor
---

Default to LZO compression for snap packages. 
This greatly improves cold startup performance (https://snapcraft.io/blog/why-lzo-was-chosen-as-the-new-compression-method). 
LZO has already been adopted by most desktop-oriented snaps outside of the Electron realm.

For the rare case where developers prefer a smaller file size (XZ) to vastly improved decompression performance (LZO),
provide an option to override the default compression method.

Consumers do not need to update their configuration unless they specifically want to stick to XZ compression.

The dependency update gets picked up automatically by the changeset bot, I presume?

Edit: om26er#4

[om26er]

Thanks @ppd for your PRs.

@maintainers does it look good now ?

[mmaietta]

Great work!
Re: updating:

- app-builder-bin: 3.7.1
+ app-builder-bin: 4.1.0

Is this a breaking electron-builder upgrade? I see this comment for app-builder, but the .changeset file shows as a minor

I published [email protected]. The reason for breaking change — Go 1.17 requires macOS 10.13 High Sierra or later. I decided that it is ok.

[ppd]

@mmaietta Making it a major version increase would make sense from the system compatibility POV. I think I thought about it in the API/config sense when I wrote the changeset; it's a non-breaking change from that perspective.

So let's make it a major changeset then?! @om26er

[om26er]

So let's make it a major changeset then?! @om26er

yeah, sounds good to me

[ppd]

@om26er om26er#5

[mmaietta]

Great to see the tests passing!

Hmmm re:

[email protected]/node_modules/app-builder-bin/linux/x64/app-builder exited with code ERR_ELECTRON_BUILDER_CANNOT_EXECUTE

I'm wondering if GO version needs to be updated in the docker images as well. Could someone please check that? I only have access to my hackintosh during these holidays, so I can't run anything related to docker atm.

@develar, can you please grant me access to our dockerhub account so I can perform this maintenance?

[ppd]

@mmaietta There's this error in the logs:

error while loading shared libraries: liblzo2.so.2

E.g. https://app.circleci.com/pipelines/github/electron-userland/electron-builder/1097/workflows/fe032184-a970-40f1-a9ee-bceaa7f9ea6c/jobs/4847/parallel-runs/2?filterBy=FAILED&invite=true#step-106-47

That probably needs to be added to the docker image.

[mmaietta]

So it seems I can't just add it to the docker image locally as it, unsurprisingly, doesn't build on my M1 mac. I can get it to build via GH Actions though.

If you're willing to try it out, as I'm unable to test locally, I think the way to fix this would be to add liblzo2-dev to the base Dockerfile

electron-builder/docker/base/Dockerfile

Lines 13 to 16 in cd79c53

	apt-get -qq install --no-install-recommends \
	qtbase5-dev build-essential autoconf libssl-dev gcc-multilib g++-multilib \
	lzip rpm python libcurl4 git git-lfs ssh unzip libarchive-tools \
	libxtst6 libsecret-1-dev libopenjp2-tools \

Then for testing the image locally, I think this should work? (Haven't worked much with our Docker setup yet)

sh docker/build.sh
TEST_FILES="snapTest" pnpm test-linux

Currently working on trying to get docker set up with GH Actions for testing dockerfile changes as part of the CI.

[ppd]

@mmaietta

Looks good:

Test Suites: 1 passed, 1 total
Tests:       13 passed, 13 total
Snapshots:   33 passed, 33 total
Time:        38.689 s
Ran all test suites matching /snapTest\.ts$/i.

with om26er#6 @om26er

[netlify]

✔️ Deploy Preview for car-park-attendant-cleat-11576 ready!

🔨 Explore the source changes: 77c00af

🔍 Inspect the deploy log: https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/61df1783f401a700077f678c

😎 Browse the preview: https://deploy-preview-6201--car-park-attendant-cleat-11576.netlify.app

[ppd]

am I blind, or does Test not run the snap tests?

[mmaietta]

I was blind. Didn't realize there were circleci tests running on an electron-builder docker image. pnpm support on circleci is pretty meh too, so I'm seeing if I can just migrate the tests to Github Actions off of circleci

The circleci tests oddly seem to be duplicating multiple Github Action tests already too. Might just be deprecated then, will investigate further to unblock this PR 🤔

[mmaietta]

I've migrated all tests to GH Actions from TravisCI and CircleCI in #6541. The TravisCI tests weren't even running 🤯

Can you please pull in latest master branch?

[om26er]

Can you please pull in latest master branch?

done, let's see how it goes

[mmaietta]

Okay okay, I think I've stabilized all tests on master now, and it should properly trigger on your PR as well now.

Please pull in latest master again (apologies on the delays here, thanks for your patience 😄 )

[om26er]

Merged with master. CI says "First-time contributors need a maintainer to approve running workflows.". That's probably needs your action

[mmaietta]

Great work guys! Really appreciate the contribution

[om26er]

Thanks @mmaietta for the review and thank you @ppd for taking this PR forward

[ppd]

Thank you, @om26er for the heavy lifting, and @mmaietta for taking care of this PR. I hope someone will step up and integrate core20 (and core22) support soon, too.

[mmaietta]

Published in 23.0.0-alpha.0 and just deployed new versions of the docker images. Enjoy!

[julian-alarcon]

This was partially adding the feature, but there are still issues: #7013