feat: Apple Silicon Universal Support #5481

lutzroeder · 2020-12-16T04:19:36Z

mmaietta

With the introduction of --universal as an arch, I think this will break as node-gyp won't understand universal.

await this.installAppDependencies(platform, arch)

https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/packager.ts#L430

It can process --arch=all, however, that doesn't work for me on Mac and it just produces the system's process.arch.

$ electron-builder node-gyp-rebuild --arch universal

Rebuild own native code

Options:
  --version   Show version number                                      [boolean]
  --help      Show help                                                [boolean]
  --platform  The target platform
                       [choices: "linux", "darwin", "win32"] [default: "darwin"]
  --arch      The target arch
             [choices: "ia32", "x64", "armv7l", "arm64", "all"] [default: "x64"]

Invalid values:
  Argument: arch, Given: "universal", Choices: "ia32", "x64", "armv7l", "arm64", "all"
error Command failed with exit code 1.

[UPDATE]
I'm starting to think we should just add a property specific to the builder config, like below. Partly because the packager logic is too tied to a specific architecture, and "universal" is technically not an architecture, but rather just combining/stitching assets into a fat binary.

    mac: {
        identity: <non-null>,
        universal: true,
        target: {
            target: 'dir',
            arch: [
                'arm64',
                'x64', 
            ]
        },

In taking this approach, the target field corresponds directly to an arch AND signs it. universal just validates that both arm64 and x64 are built, but doesn't force each individually to get signed, only the final fat app.

packages/app-builder-lib/src/macPackager.ts

quanglam2807 · 2020-12-17T09:32:23Z

I have a better idea for this. Why don’t we introduce an option in build config. Something like combine: true. And if that is set, we will combine the arch-specific files together with lipo.

I also agree with @mmaietta that universal binaries are only useful for initial downloads. For updates, electron-updater should just download arch-specific files.

If you go with this design, it will be much simpler as only need to add a hook to run after arch-specific builds are ready.

mmaietta · 2020-12-17T14:17:22Z

If you go with this design, it will be much simpler as only need to add a hook to run after arch-specific builds are ready.

Perhaps a hook over here? https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/packager.ts#L442
The only difficulty I'm running into is how to map packager.pack(...) => Promise<string> so that we return a target-arch-outputpath map/object of assets to lipo after (it doesn't return anything right now). Then it'd be easy to just pass that into the hook for postProcessPackages(...)

lutzroeder · 2020-12-17T16:09:18Z

@quanglam2807 @mmaietta to get on the same page on this, there aren't apps out there shipping universal bits and then upgrading to a platform specific version. Download and update any universal app, run an update and the result will be universal. With the approach you describe the platform specific targets would need to be signed, notarized and published (on GitHub Releases) which would make CI pipelines more fragile and add complexity (now there are three targets for users to choose from instead of two, users at different stages might rund a different variants of the app). I'm not trying to argue which sets of pros and cons are preferable, more "only ever ship universal" is how it seems to be done by pretty much all app vendors so the goal is to optimize for this model.

lutzroeder · 2020-12-17T16:15:33Z

Perhaps a hook over here? https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/packager.ts#L442

What would this hook do? Create another MacPackager that doesn't run doPack, can take inputs and a universal architecture to create universal targets. Then special case MacPackager so it doesn't sign or produce targets for x64 and arm64 if universal is set? Might be easier to have another PR to have a look?

mmaietta · 2020-12-17T16:49:41Z

Thanks so much for tackling this @lutzroeder! The work is fantastic and covers almost all the use cases. The main issue lies with native modules as installAppDependencies must be called prior to any packing and per-architecture. universal unfortunately isn't accepted by node-gyp itself (all is though). I think we shouldn't move installAppDependencies into doPack as it convolutes the build stage's description of just "packing" (I suppose a simple rename could change that explicit description though, perhaps buildPackage?)
I think moving installAppDependencies here might solve the issue such that doPack always installs/rebuilds the correct arch's native addons:

electron-builder/packages/app-builder-lib/src/platformPackager.ts

Lines 161 to 162 in 70708a4

    
           protected async doPack(outDir: string, appOutDir: string, platformName: ElectronPlatformName, arch: Arch, platformSpecificBuildOptions: DC, targets: Array<Target>, sign: boolean = true) { 
        
             if (this.packagerOptions.prepackaged != null) {

Would you mind giving that a try? Installing sqlite3 or leveldown on electron-quick-start should create a simple env to test in.

I think long-term, it could be useful for each already-instantiated packager to have a postProcessPackages hook for use cases like this as I feel like we may be missing something here. Also, universal as an architecture confuses me, but the scope of going another route might be too large. I just want to make sure whatever changes we make here to the electron-builder build config is something that is flexible to change/refactor in the future.

lutzroeder · 2020-12-17T22:49:26Z

@mmaietta updated to call installAppDependencies from doPack:

~/electron-quick-start: npx electron-builder --mac --universal
  • electron-builder  version=22.10.0 os=20.2.0
  • writing effective config  file=dist/builder-effective-config.yaml
  • rebuilding native dependencies  [email protected] platform=darwin arch=x64
  • packaging       platform=darwin arch=x64 electron=11.1.0 appOutDir=dist/mac-universal-x64
  • default Electron icon is used  reason=application icon is not set
  • rebuilding native dependencies  [email protected] platform=darwin arch=arm64
  • packaging       platform=darwin arch=arm64 electron=11.1.0 appOutDir=dist/mac-universal-arm64
  • packaging       platform=darwin arch=universal electron=11.1.0 appOutDir=dist/mac-universal

mmaietta · 2020-12-18T01:05:38Z

Nice! Those logs look perfect.

For the interim, I think this is a good way to approach it without very large changes for a single hook. A larger refactor without an explicit need for a post-all-packed hook is just asking for bugs to be created. I'm still wary of the "universal" as its own arch as anyone building all 3 archs (x64, arm64, universal) with native addons rebuilding may take a very long time. Oh well 😅

I gave it a whirl locally and it works perfect on the first run. lipo results:

$ find dist/artifacts build -name '*.node' | xargs -I {} lipo -info "{}"
Architectures in the fat file: dist/artifacts/local/mac-universal/Electron.app/Contents/MacOS/app.node are: x86_64 arm64

It does throw an error on subsequent builds that is an easy fix:

  • packaging       platform=darwin arch=arm64 electron=11.0.3 appOutDir=dist/artifacts/local/mac-universal-arm64
  • packaging       platform=darwin arch=universal electron=11.0.3 appOutDir=dist/artifacts/local/mac-universal
  ⨯ The out path "/shell-builder/dist/artifacts/local/mac-universal/Electron.app" already exists and force is not set to true

Just need to add force: true to the makeUniversalApp config to enable overwriting

The only remaining step AFAICT is to get this working with the electron-builder config so that this works:

    mac: {
        target: {
            target: 'dir',
            arch: 'universal'
        },

lutzroeder · 2020-12-18T01:25:31Z

The only remaining step AFAICT is to get this working with the electron-builder config so that this works

This config gives Invalid configuration object. electron-builder 22.10.0 has been initialized using a configuration object that does not match the API schema.?

mmaietta · 2020-12-18T01:31:40Z

Yep! You'll need to update the scheme.json. Just gotta add "universal", here:
https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/scheme.json#L4955-L4980

Now, how can we fix that test?

lutzroeder · 2020-12-18T01:42:30Z

Now, how can we fix that test?

That should probably be a separate PR as it happens for blank PRs like #5488 as well. We should fix the test first and merge vs. including it in this PR which has higher risk to be reverted. Can you have a look?

mmaietta · 2020-12-18T03:27:18Z

Your tests pass here!
But locally they fail for me, I get:

Snapshot name: `two-package 1`

    - Snapshot  - 3
    + Received  + 3

    @@ -68,21 +68,21 @@
                  "url": "TestApp-1.1.0-mac-universal.zip",
                },
                Object {
                  "sha512": "@sha512",
                  "size": "@size",
    -             "url": "TestApp-1.1.0-mac.dmg",
    +             "url": "TestApp-1.1.0-mac-universal.dmg",
                },
                Object {
                  "sha512": "@sha512",
                  "size": "@size",
    -             "url": "TestApp-1.1.0-mac-arm64.dmg",
    +             "url": "TestApp-1.1.0-mac.dmg",
                },
                Object {
                  "sha512": "@sha512",
                  "size": "@size",
    -             "url": "TestApp-1.1.0-mac-universal.dmg",
    +             "url": "TestApp-1.1.0-mac-arm64.dmg",
                },
              ],
              "path": "TestApp-1.1.0-mac.zip",
              "releaseDate": "@releaseDate",
              "sha512": "@sha512",

      235 |   }
      236 | 
    > 237 |   expect(objectToCompare).toMatchSnapshot()

I feel like something is wrong with the build node as these results are way too inconsistent.
Yours fllows this order from packages/electron-builder/src/builder.ts [Arch.x64, Arch.arm64, Arch.universal], whereas mine is in some weird format.

[UPDATE]
I figured out the discrepancy issue. Node 12 fails certain tests for me. I installed Node 14 (same as build machine) and they pass.

lutzroeder · 2020-12-18T04:16:58Z

@mmaietta not sure how this explains #5488 failing? Is there sorting needed after result.fileContent = removeUnstableProperties(safeLoad(await fs.readFile(file, "utf-8")))?

quanglam2807 · 2020-12-18T15:21:43Z

Thank you so much, @lutzroeder. I'm so excited for this. I just realize Mac App Store only accepts a single pkg file, so universal binary is a must. Hope this PR can be merged soon so we can then test in production.

lutzroeder · 2020-12-18T16:37:09Z

@mmaietta @develar can we merge this PR and #5488 for another round of prerelease testing?

mmaietta

Fantastic work @lutzroeder! 🎉

packages/app-builder-lib/src/packager.ts

mmaietta · 2020-12-18T22:44:10Z

I think we should also merge these for the prerelease cut:
#5488 #5489 #5484
CC @quanglam2807

@lutzroeder I, unfortunately, don't have merge permissions for this project. It'd be awesome if I did though!

mmaietta · 2020-12-21T03:54:45Z

Quick follow up, can you please update this PR's yarn.lock 🙂

Also, @lutzroeder I've pushed a "PreRelease" branch to my fork containing multiple fixes from open PRs we've worked on (dmg, universal, signing, unit tests). Feel free to generate patch files from it to use for the interim.
https://github.com/mmaietta/electron-builder/tree/PreRelease

Just compile, link, and use rsync to migrate all the files so you can run patch-package on your project.
Handy cmd: rsync -avh --exclude=package.json --exclude=yalc.sig .yalc/ node_modules/
You'll need to manually yarn add -D istextorbinary @electron/universal in your project for now because yarn install can't utilize a patched package.json to update the yarn.lock.

develar · 2020-12-21T10:22:04Z

Thanks for your work, you are awesome :)

I cannot merge #5488 since not clear for me, is it safe just revert or it is possible just add required patterns to auto sign (#5488 (comment)).

lutzroeder · 2020-12-21T12:29:27Z

Quick follow up, can you please update this PR's yarn.lock

@mmaietta ran yarn install and added the updated yarn.lock. Is that all that is needed for this PR?

mmaietta

LGTM

lutzroeder · 2020-12-22T17:42:12Z

@develar @mmaietta what does it take to push a new prerelease with the recent changes?

Is there a doc page how to build electron-builder locally and fully patch the node_modules of a project?

mmaietta · 2020-12-22T19:10:25Z

I have no idea in terms of a release 😅

In terms of patch package for tracking latest master, here's how I've done it:
Step 1

yarn compile && \
(cd ./packages/app-builder-lib && yalc push) && \
(cd ./packages/builder-util && yalc push) && \
(cd ./packages/builder-util-runtime && yalc push) && \
(cd ./packages/dmg-builder && yalc push) && \
(cd ./packages/electron-builder && yalc push) && \
(cd ./packages/electron-publish && yalc push) && \
(cd ./packages/electron-builder-squirrel-windows && yalc push) && \
(cd ./packages/electron-forge-maker-appimage && yalc push) && \
(cd ./packages/electron-forge-maker-nsis && yalc push) && \
(cd ./packages/electron-forge-maker-nsis-web && yalc push) && \
(cd ./packages/electron-forge-maker-snap && yalc push) && \
(cd ./packages/electron-updater && yalc push)

Step 2 in your project:

yalc remove --all
yalc retreat --all
yarn install --check-files --force --ignore-scripts
yalc link app-builder-lib builder-util builder-util-runtime dmg-builder electron-builder electron-publish electron-builder-squirrel-windows electron-forge-maker-appimage electron-forge-maker-nsis electron-forge-maker-nsis-web electron-forge-maker-snap electron-updater
rsync -avh --exclude=package.json --exclude=yalc.sig .yalc/ node_modules/
yarn patch-package app-builder-lib --exclude
yarn patch-package builder-util --exclude
yarn patch-package electron-builder --exclude
yarn patch-package electron-updater --exclude

If you hit a permission issue during yarn install, then just rerun the retreat and remove commands until it properly resets the permissions in node_modules. Note: the --ignore-scripts is necessary to avoid applying your current patch files as it'll break the subsequent attempts to create new patches.

A doc page for that above would probably be best in the yalc repo, but I suppose it'd still be super handy in our CONTRIBUTING.md file. Can you give commands above a try to test it outside of my local env? If successful, then we could consider updating an EB doc

develar · 2020-12-22T19:34:35Z

22.10.4 is released as pre-release.

mmaietta · 2020-12-22T20:07:53Z

@lutzroeder EB provides an electronDist param for custom electron dists/forks, which is what my team uses and something I think we may have overlooked. Not a bug as part of this PR as this issue already exists for compiling 2+ archs on a single build run, as there will always be only one path to an arch-specific electron dist.

For building universal, we need to provide 2 different paths, one for each arch. However, electronDist only accepts one path to a corresponding .app.
What are your thoughts on us enabling electronDist to accept a dictionary of ${platform}-${arch} : <path> key-value pairs? It'd retain backward compatibility while enabling custom forks for universal builds.

CC @quanglam2807

lutzroeder · 2020-12-23T02:13:43Z

@quanglam2807 what is causing them to be different?

quanglam2807 · 2020-12-23T02:15:11Z

@quanglam2807 what is causing them to be different?

We generate app.asar for each arch-specific build separately so they're different files with different SHAs.

lutzroeder · 2020-12-23T02:25:37Z

@quanglam2807 do the app.asar files contain arch-specific binaries or do they contain something else that would make them different? If the difference is arch-specific binaries not sure how that would work as lipo is expected to merge such binaries.

lutzroeder · 2020-12-23T02:26:12Z

Follow-up #5495

quanglam2807 · 2020-12-23T03:00:22Z

@quanglam2807 do the app.asar files contain arch-specific binaries or do they contain something else that would make them different? If the difference is arch-specific binaries not sure how that would work as lipo is expected to merge such binaries.

app.asar normally doesn't contain binaries. If it does, electron-builder will automatically unpack them from the asar file.

We generate x64 & arm64 builds separately before merging them together. So for each arch, we generate different app.asar file. These files are identical in contents, but could have different SHA because they differ in metadata (eg date created).

So in this case, we can opt to remove AsarIntegrity from Info.plist. Or we can unify the app.asar process for universal build.

lutzroeder · 2020-12-23T03:54:44Z

@quanglam2807 do you have a repro project or is there some extra logging that needs to be enabled? I've been running with 20.10.4, pushed a release and haven't seen this error.

quanglam2807 · 2020-12-23T03:57:29Z

@lutzroeder You can clone this repo: https://github.com/webcatalog/translatium

Then remove the patch: patches/app-builder-lib+22.10.4.patch. Run yarn and then yarn dist-dev.

vadim-termius · 2020-12-30T02:45:53Z

packages/app-builder-lib/src/macPackager.ts

+  protected async doPack(outDir: string, appOutDir: string, platformName: ElectronPlatformName, arch: Arch, platformSpecificBuildOptions: MacConfiguration, targets: Array<Target>): Promise<any> {
+    switch (arch) {
+      default: {
+        return super.doPack(outDir, appOutDir, this.platform.nodeName as ElectronPlatformName, arch, this.platformSpecificBuildOptions, targets);


Hi @lutzroeder,

Thank you for the universal support.

Why do you overwrite platformName and platformSpecificBuildOptions instead of passing arguments directly? It breaks mas builds.

Good catch. Can you create an issue for tracking, link #5511 and review the change?

fabiospampinato · 2021-01-16T17:47:33Z

Does this PR take into account electron-updater?

Because when I switch from the x64 to the universal architecture I'm not getting the build artifacts that I would need, just a single file at mac-universal/MyApp.app and this error in the terminal:

Error: spawn zip ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:264:19)
    at onErrorNT (internal/child_process.js:456:16)
    at processTicksAndRejections (internal/process/task_queues.js:80:21)

I'm under Mojave.

lutzroeder · 2021-01-16T18:43:19Z

@fabiospampinato see #5504 which is not included in a preview build yet and #5495.

fabiospampinato · 2021-01-16T20:17:30Z

@lutzroeder Would that probably address my issue? 🤔 I can get a universal build of the app, but I can't get the dmg and accessory files. Seeing as an error, possibly related to packing the app, got thrown at build time I'm not sure #5504 would address my issue.

lutzroeder changed the title ~~Apple Silicon Universal Support~~ feat: Apple Silicon Universal Support Dec 16, 2020

mmaietta reviewed Dec 17, 2020

View reviewed changes

packages/app-builder-lib/src/macPackager.ts Show resolved Hide resolved

lutzroeder marked this pull request as ready for review December 17, 2020 05:28

mmaietta approved these changes Dec 18, 2020

View reviewed changes

packages/app-builder-lib/src/packager.ts Show resolved Hide resolved

feat: Apple Silicon Universal Support

551de65

mmaietta approved these changes Dec 21, 2020

View reviewed changes

develar merged commit ca20151 into electron-userland:master Dec 22, 2020

vladimiry mentioned this pull request Dec 22, 2020

Apple Silicon/M1 Support vladimiry/ElectronMail#357

Closed

vadim-termius reviewed Dec 30, 2020

View reviewed changes

lutzroeder mentioned this pull request Dec 30, 2020

fix(mac): Pass platformName and options in doPack #5511

Merged

mmaietta mentioned this pull request Jan 7, 2021

feat: Custom electronDist callback #5527

Merged

quanglam2807 mentioned this pull request Jan 12, 2021

Plans to Support Apple Silicon builds? #5392

Closed

csett86 mentioned this pull request Jan 16, 2021

Replace previous binary with n-api native module karaggeorge/mac-screen-capture-permissions#6

Merged

mmaietta mentioned this pull request Jan 17, 2021

Can't create a universal build #5547

Closed

gnattu mentioned this pull request Jan 20, 2021

Apple Silicon Universal Build AdguardTeam/AdGuardForSafari#493

Closed

EtienneLem mentioned this pull request Feb 17, 2021

Apple Silicon can’t auto-update with Universal target #5639

Closed

Ch00k mentioned this pull request Feb 23, 2021

Add support for Apple ARM64 (aka Apple Silicon) mullvad/mullvadvpn-app#2500

Merged

This was referenced Mar 7, 2021

Bump electron-builder from 22.9.1 to 22.10.5 in /electron wireapp/wire-webapp#10666

Merged

Bump electron-builder from 22.9.1 to 22.10.5 Threor/soundboard-frontend#23

Merged

septs mentioned this pull request Sep 17, 2021

feat: apple silicon universal support RocketChat/Rocket.Chat.Electron#2170

Merged

Keith-CY mentioned this pull request Oct 8, 2022

Apple Silicon support Magickbase/neuron-public-issues#62

Closed

undergroundwires mentioned this pull request Apr 28, 2024

Apple silicon processor support undergroundwires/privacy.sexy#348

Closed

feat: Apple Silicon Universal Support #5481

feat: Apple Silicon Universal Support #5481

Conversation

lutzroeder commented Dec 16, 2020 • edited Loading

mmaietta left a comment • edited Loading

Choose a reason for hiding this comment

quanglam2807 commented Dec 17, 2020

mmaietta commented Dec 17, 2020

lutzroeder commented Dec 17, 2020 • edited Loading

lutzroeder commented Dec 17, 2020 • edited Loading

mmaietta commented Dec 17, 2020 • edited Loading

lutzroeder commented Dec 17, 2020

mmaietta commented Dec 18, 2020

lutzroeder commented Dec 18, 2020 • edited Loading

mmaietta commented Dec 18, 2020

lutzroeder commented Dec 18, 2020

mmaietta commented Dec 18, 2020 • edited Loading

lutzroeder commented Dec 18, 2020 • edited Loading

quanglam2807 commented Dec 18, 2020

lutzroeder commented Dec 18, 2020 • edited Loading

mmaietta left a comment

Choose a reason for hiding this comment

mmaietta commented Dec 18, 2020

mmaietta commented Dec 21, 2020

develar commented Dec 21, 2020 • edited Loading

lutzroeder commented Dec 21, 2020

mmaietta left a comment

Choose a reason for hiding this comment

lutzroeder commented Dec 22, 2020

mmaietta commented Dec 22, 2020 • edited Loading

develar commented Dec 22, 2020

mmaietta commented Dec 22, 2020

lutzroeder commented Dec 23, 2020

quanglam2807 commented Dec 23, 2020

lutzroeder commented Dec 23, 2020 • edited Loading

lutzroeder commented Dec 23, 2020

quanglam2807 commented Dec 23, 2020

lutzroeder commented Dec 23, 2020 • edited Loading

quanglam2807 commented Dec 23, 2020

vadim-termius Dec 30, 2020

Choose a reason for hiding this comment

lutzroeder Dec 30, 2020 • edited Loading

Choose a reason for hiding this comment

fabiospampinato commented Jan 16, 2021

lutzroeder commented Jan 16, 2021

fabiospampinato commented Jan 16, 2021 • edited Loading

lutzroeder commented Dec 16, 2020 •

edited

Loading

mmaietta left a comment •

edited

Loading

lutzroeder commented Dec 17, 2020 •

edited

Loading

lutzroeder commented Dec 17, 2020 •

edited

Loading

mmaietta commented Dec 17, 2020 •

edited

Loading

lutzroeder commented Dec 18, 2020 •

edited

Loading

mmaietta commented Dec 18, 2020 •

edited

Loading

lutzroeder commented Dec 18, 2020 •

edited

Loading

lutzroeder commented Dec 18, 2020 •

edited

Loading

develar commented Dec 21, 2020 •

edited

Loading

mmaietta commented Dec 22, 2020 •

edited

Loading

lutzroeder commented Dec 23, 2020 •

edited

Loading

lutzroeder commented Dec 23, 2020 •

edited

Loading

lutzroeder Dec 30, 2020 •

edited

Loading

fabiospampinato commented Jan 16, 2021 •

edited

Loading