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

feat(snap): add Snap compression option #6201

Merged
merged 8 commits into from
Jan 12, 2022

Conversation

om26er
Copy link
Contributor

@om26er om26er commented Aug 28, 2021

Fixes #5881

Signed-off-by: Omer Akram om26er@gmail.com

@changeset-bot
Copy link

changeset-bot bot commented Aug 28, 2021

🦋 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
Copy link
Contributor Author

om26er commented Aug 29, 2021

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
Copy link
Contributor Author

om26er commented Aug 29, 2021

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
Copy link
Member

develar commented Aug 29, 2021

Thanks! I published app-builder-bin@4.0.0. The reason for breaking change — Go 1.17 requires macOS 10.13 High Sierra or later. I decided that it is ok.

@om26er
Copy link
Contributor Author

om26er commented Sep 2, 2021

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

@om26er
Copy link
Contributor Author

om26er commented Sep 5, 2021

@ppd
Copy link
Contributor

ppd commented Oct 11, 2021

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

@ppd
Copy link
Contributor

ppd commented Nov 28, 2021

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:

"type": "null"
}
],
"default": "xz",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

om26er pushed a commit to om26er/electron-builder that referenced this pull request Dec 2, 2021
* use new app-builder-bin & hook up compression option

* set default snap compression type to lzo
@ppd
Copy link
Contributor

ppd commented Dec 6, 2021

@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
Copy link
Contributor

ppd commented Dec 7, 2021

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

/root/project/node_modules/.pnpm/app-builder-bin@4.1.0/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
Copy link
Contributor

ppd commented Dec 8, 2021

@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
Copy link
Contributor Author

om26er commented Dec 22, 2021

Thanks @ppd for your PRs.

@maintainers does it look good now ?

@mmaietta
Copy link
Collaborator

mmaietta commented Dec 28, 2021

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 app-builder-bin@4.0.0. The reason for breaking change — Go 1.17 requires macOS 10.13 High Sierra or later. I decided that it is ok.

@ppd
Copy link
Contributor

ppd commented Dec 29, 2021

@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
Copy link
Contributor Author

om26er commented Dec 29, 2021

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

yeah, sounds good to me

@ppd
Copy link
Contributor

ppd commented Dec 29, 2021

@om26er om26er#5

@mmaietta
Copy link
Collaborator

mmaietta commented Jan 2, 2022

Great to see the tests passing!

Hmmm re:

app-builder-bin@4.1.0/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
Copy link
Contributor

ppd commented Jan 2, 2022

@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
Copy link
Collaborator

mmaietta commented Jan 8, 2022

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

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
Copy link
Contributor

ppd commented Jan 9, 2022

@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
Copy link

netlify bot commented Jan 9, 2022

✔️ 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
Copy link
Contributor

ppd commented Jan 10, 2022

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

@mmaietta
Copy link
Collaborator

mmaietta commented Jan 10, 2022

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
Copy link
Collaborator

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 pushed a commit to om26er/electron-builder that referenced this pull request Jan 11, 2022
* use new app-builder-bin & hook up compression option

* set default snap compression type to lzo
@om26er
Copy link
Contributor Author

om26er commented Jan 11, 2022

Can you please pull in latest master branch?

done, let's see how it goes

@mmaietta mmaietta changed the base branch from v23.0.0-alpha to master January 11, 2022 22:52
@mmaietta
Copy link
Collaborator

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 and others added 8 commits January 12, 2022 23:01
Signed-off-by: Omer Akram <om26er@gmail.com>
Signed-off-by: Omer Akram <om26er@gmail.com>
* use new app-builder-bin & hook up compression option

* set default snap compression type to lzo
* make lzo the compression default in snap options

* add lzo  compression to snapcraft template

* construct compression arg from descriptor

* add tests for snap compression option
@om26er
Copy link
Contributor Author

om26er commented Jan 12, 2022

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

@mmaietta mmaietta changed the base branch from master to v23.0.0-alpha January 12, 2022 19:09
@mmaietta mmaietta merged commit c5f8b08 into electron-userland:v23.0.0-alpha Jan 12, 2022
@mmaietta
Copy link
Collaborator

Great work guys! Really appreciate the contribution

@om26er
Copy link
Contributor Author

om26er commented Jan 12, 2022

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

@ppd
Copy link
Contributor

ppd commented Jan 12, 2022

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 mmaietta mentioned this pull request Jan 16, 2022
mmaietta added a commit that referenced this pull request Jan 16, 2022
* Adding INPUTxxx and OUTPUTxxx CHARSETS to makensis
Fixes: #4898 #6232 #6259

* Adding additional details to error console logging

* Breaking change: Removing Bintray support since it was sunset. Ref: https://jfrog.com/blog/into-the-sunset-bintray-jcenter-gocenter-and-chartcenter/

* fix: force strip path separators for backslashes on Windows

* fix: Force authentication for local Mac Squirrel update server

* Breaking Change: Fail-fast for signature verification failures. Adding `-LiteralPath` to update file for injected wildcards

* Adding changeset and eslint

* Fix error: `-OUTPUTCHARSET is disabled for non Win32 platforms.`

* feat(mac): ElectronAsarIntegrity in electron@15 (#6511)

* feat(mac): ElectronAsarIntegrity in electron@15
See: electron/electron#30667
Fix: #6506
Fix: #6507

* fix(msi): MSI fails to install when deployed machine-wide via GPO (#6514)

* fix(msi): MSI fails to install when deployed machine-wide via GPO
* Disable advertised shortcuts, since MSIs with advertised Start Menu shortcuts that have a
Shortcut Property fails to install when deployed machine-wide via GPO but works fine in all
other contexts. This might be a bug in Windows or a misdiagnosis; see #6508 for more details.
Closes #6508
* BREAKING CHANGE: Admins using advertisement must apply an MST to re-enable it. See #6508.

* Don't set GitHub Releases draft title since it automatically pulls it from tag name. Fixes #3683

* feat(snap): add lzo to Snap compression options (also as new default) (#6201)

* feat(msi): add fileAssociation support for MSI target (#6530)

* fix(win): iconId sometimes containing invalid characters, and iconId config option being ignored.
* fix(msi): change the fallback value for generated MSI Ids to a unique string for the product.

* BREAKING CHANGE: remove MSI option `iconId`

* fix: stabilizing tests by moving updater tests to its own node to explicitly segment env.___TOKEN integration tests from other standard unit tests

* chore: synchronizing docs and schema plus prettier

* Adding changset to set as alpha

* Updating changeset documentation

* feat(msi): support assisted installer for MSI target (#6550)

* Add basic support for assisted installer, with UI to choose between per-user and per-machine. Supported config settings: runAfterFinish, perMachine, oneClick. Not supported: license (EULA), allowToChangeInstallationDirectory, etc. Also prevent oneClick's runAfterFinish from executing when installed silently.

Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com>
Co-authored-by: Alex Plumley <alex+git@aplum.ca>
Co-authored-by: Mike Maietta <mmaietta@outlook.com>
Co-authored-by: Omer Akram <om26er@gmail.com>
Co-authored-by: Maximilian Federle <max.federle@gmail.com>
@mmaietta
Copy link
Collaborator

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

mergify bot pushed a commit to enso-org/enso that referenced this pull request Apr 21, 2022
[ci no changelog needed]

[Task link](https://www.pivotaltracker.com/story/show/181944234).

It fixes the build issue on Mac OS 12.3.1 that is caused by removed `/usr/bin/python` executable.

Also applied `enso-formatter` to the sources.

# Important Notes
We're basically updating for one major `electron-builder` release - from `v22` to `v23`. I didn't spot anything in the changelog that could affect us. See features + breaking changes excerpt:

```
Features:

- feat(msi): add fileAssociation support for MSI target (electron-userland/electron-builder#6530)
- feat(mac): ElectronAsarIntegrity in electron@15 - See: electron/electron#30667 (electron-userland/electron-builder#6506 electron-userland/electron-builder#6507)
- feat(snap): add lzo to Snap compression options (also as new default) (electron-userland/electron-builder#6201) Upgraded app-builder-bin dependency required newer version of Go
- feat(msi): support assisted installer for MSI target (electron-userland/electron-builder#6550)

Breaking changes:

- Removing Bintray support since it was sunset. Ref: https://jfrog.com/blog/into-the-sunset-bintray-jcenter-gocenter-and-chartcenter/
- Fail-fast for windows signature verification failures. Adding -LiteralPath to update file path to disregard injected wildcards
- Force strip path separators for backslashes on Windows during update process
- Authentication for local mac squirrel update server
- Disabled advertised shortcuts, since MSIs with advertised Start Menu shortcuts that have a
Shortcut Property fails to install when deployed machine-wide via GPO but works fine in all
other contexts. Admins using advertisement must apply an MST to re-enable it. See electron-userland/electron-builder#6508.
- Removing optional NSIS icon ID from config and generating it automatically to synchronize IDs with Advertised Shortcuts and future features
```
@julian-alarcon
Copy link

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

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

Successfully merging this pull request may close these issues.

Add support for snapcraft compression top-level key
5 participants