-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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:(mac): Apple Silicon builds (arm64) #5426
feat:(mac): Apple Silicon builds (arm64) #5426
Conversation
… hardcoded values and making sure user-options are utilized for platform/dmg packagers. Updated tests to target electron 11. Updated mac snapshots to include multi-arch images
d841e1e
to
1acc32e
Compare
@mmaietta Do you have any thoughts on next steps to support building a single universal binary? |
That actually depends on Source: https://www.electronjs.org/blog/apple-silicon
|
@quanglam2807 see electron/electron#26710. The package already exists. |
Yes, the build and tests were passing previously, but master branch is currently broken. I'm hesitant to merge into something broken (currently working on this PR #5449 to fix master, almost done). If you'd like to merge now, then we can do so. |
Just in case someone else hits this, one thing I've found is that I get an error if I run
I think the reason for this is that the From reading this link - I think this has something to do with the Anyway, running the command with |
I think there's a bug with the The current findFile seems to use the first matching-suffix file it finds. So both arm64 and x86 will download whichever is listed first in the Because
They're both
Or do we need a |
@@ -16,7 +16,7 @@ test.ifMac.ifAll("two-package", () => assertPack("test-app", { | |||
electronLanguages: ["bn", "en"] | |||
}, | |||
//tslint:disable-next-line:no-invalid-template-strings | |||
artifactName: "${name}-${version}-${os}.${ext}", | |||
artifactName: "${name}-${version}-${os}-${arch}.${ext}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my testing I think this is:
artifactName: "${name}-${version}- ${arch}-${os} .${ext}",
Seems to be generated in https://github.com/electron-userland/electron-builder/blob/master/packages/app-builder-lib/src/targets/ArchiveTarget.ts#L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that is true if an artifactName
template is not provided in the build config. I assumed this test was also validating artifactName
as opposed to using the defaultPattern
you linked in ArchiveTarget.ts.
I updated this test to have the ${arch}
template explicitly specified as otherwise, the dmg/zip will overwrite itself when building for multiple targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, maybe so.
I do think I've been seeing something like this. When I'm exporting and uploading DMGs, the -arm64 DMGs don't get published - it's weird. It doesn't seem to happen every time, but I can't figure out what's causing it to happen.
Nice callout. I see electron-builder/packages/electron-updater/src/providers/Provider.ts Lines 149 to 152 in f938de5
My worry is if/how we need to migrate between electron-updater schema versions? What are your thoughts? I'm really not familiar with how electron-updater works as my team doesn't use it. |
That sounds good. Might also be nice to refactor the overwriting results[0] thing to just return a single file. What about if we patched MacUpdater with the
Sorry, I don't really know what you mean by this. I don't use it very much either (I just use and enjoy electron-builder) but I'm sure we could deal with that if/when it comes up. What's the concern? |
…64 does not have a bitness prefix to filter by)
Completely agreed. Updated the PR accordingly.
I have no idea how to handle universal binaries as the build process is outside of electron-builder, so I have no idea how it'll integrate with electron-updater. It should be outside the scope of this PR though since that would require an entirely new npm module integrated Hopefully we can get a pre-release cut first :) |
From my perspective: universal binary simply combines x64 and arm64 .app files into a single one. So technically, when a user runs the app, they still runs the same individual x64 or arm64 binary. Because of this, it wouldn’t break electron-updater as it still relies on arch-specific builds to update. |
I'm having trouble with DMG filenames. I'm running them locally, as well as in a GH Action to publish. Sometimes they come out the same, without the ${arch}, sometimes they don't. It feels like a weird race condition, or maybe I've forgotten a command somewhere. It seems to happen consistently on GH Actions, where I'm using Update: I figured it out. I was using |
My 2c on universal electron binaries is that they'll be HUGE. It might just be easier to have two download buttons, at least for now? |
@elliottkember all 3 options should be supported: arm64, x86_64 and universal. The bits might be larger but another consideration is the complexity of choosing the right architecture. For some apps users shouldn't be exposed to this even if it means larger binaries. As discussed already auto-update is another aspect. Not so much now, but in the future when Intel is phased out apps might have to keep the |
@lutzroeder That's a good point. I'm working with a 500MB app and it's easy to forget that there are many smaller apps around now. Universal is definitely preferable. |
Everything is working correctly for |
Are you running on Big Sur or Catalina, does Terminal have full disk access? See #5322. |
I build using GitHub Actions, it's probably Catalina. |
@mmaietta Found it:
|
I renew my comment here #5484
|
Has anyone confirmed that the |
@mmaietta is there a fix for the Travis CI failure in |
@lutzroeder unfortunately, I'm not too sure. Oddly, all 3 of my PRs had all tests passing prior to merging. |
@mmaietta Makes sense. Previously this was |
Yeah, that's most likely. Oddly, it seems that it's already trying to: The test that is broken in your PR ( |
The sorting happens at the It is also failing on "blank" PRs like #5488. |
@larrywalters I'm seeing the same issue, my DMG artifact name is: Would be interesting to know if others are experiencing similar and if this is a known issue. Not difficult to rename the file after Electron Builder has done its magic but I don't want to do anything if this will be "fixed" later. |
@develar @mmaietta @lutzroeder @quanglam2807 I'm happy to try and PR for the issue above (broken artifact names for DMG) but before I jump in, could you verify that:
|
1 similar comment
@develar @mmaietta @lutzroeder @quanglam2807 I'm happy to try and PR for the issue above (broken artifact names for DMG) but before I jump in, could you verify that:
|
@davej Thanks for sharing, that's interesting because when I tried it with DMG it worked fine. In fact, it also seemed to work fine for all the zipped formats I tried, but not for PKG. Therefore, I concluded it was an issue specifically with PKG. In looking at the code, I see that the call to |
@larrywalters Interesting. The zip outputs work fine for me too and I haven't tried pkg. {
"dmg": {
"artifactName": "${productName} ${version}-${arch}.${ext}"
},
"mac": {
"artifactName": "${productName} ${version}-${arch}-${os}-update.${ext}",
// ...
"target": [
{
"target": "zip",
"arch": [
"x64",
"arm64"
]
},
{
"target": "dmg",
"arch": [
"x64",
"arm64"
]
}
],
}
} And my terminal output looks like this:
|
@mmaietta @quanglam2807 looks like pkg.ts changes are not included? |
mas-related changes are here: #5484. It works fine for me (using it in production). |
@davej Please pardon the delay. I tried it for example with this configuration. It works for all the formats there except pkg. I have tried other zip formats in the past successfully too.
The output was like below; only one pkg file appeared and it wrapped the arm64 version, I think suspect means one architecture overwrote the other.
So I think we need that |
This commit introduces a universal binary format in the distributed MDG files for macOS, improving support for both Apple Silicon (ARM) and Intel (x64) architectures. It uses `electron-builder` to package both architectures into a single executable, ensuring the application can natively on any macOS hardware without depending on the GitHub runners' architecture. It fixes the issue related to prior releases that supported only the architecture of the build environment itself, which is subject to change. Changes: - Update DMG distribution to include both ARM64 and x64 architectures. - Enhance system requirements documentation to reflect support for both architectures. - Modify CI/CD workflows to check desktop runtime errors for both ARM64 and x64 versions on macOS. Resolves: - Issue #348: Initial request for Apple Silicon support. - Issue #362: Correction of distribution limited to ARM64 in release 0.13.3. `electron-builder` support: - electron-userland/electron-builder#5475 - electron-userland/electron-builder#5689 - electron-userland/electron-builder#5426
This commit introduces a universal binary format in the distributed MDG files for macOS, improving support for both Apple Silicon (ARM) and Intel (x64) architectures. It uses `electron-builder` to package both architectures into a single executable, ensuring the application can natively on any macOS hardware without depending on the GitHub runners' architecture. It fixes the issue related to prior releases that supported only the architecture of the build environment itself, which is subject to change. Changes: - Update DMG distribution to include both ARM64 and x64 architectures. - Enhance system requirements documentation to reflect support for both architectures. - Modify CI/CD workflows to check desktop runtime errors for both ARM64 and x64 versions on macOS. Resolves: - Issue #348: Initial request for Apple Silicon support. - Issue #362: Correction of distribution limited to ARM64 in release 0.13.3. `electron-builder` support: - electron-userland/electron-builder#5475 - electron-userland/electron-builder#5689 - electron-userland/electron-builder#5426
Apple Silicon support: Initial dev work for achieving arm64 builds for Mac. This PR enables packaging both arm64 and x64 on mac. Supports building native app dependencies for both archs as well. dmg assets will follow current electron-builder logic/nomenclature, with only x64 builds not having an arch suffix.
Opening PR for tracking issues:
#5095
#5392
#5397
#5402
Quick Notes:
Unrelated, PR also fixes build error with unused
semver
importCopying here from #5392 for posterity's sake. I'm unable test on MacOS Big Sur as I'm blocked from installing it on my company laptop. I'd appreciate it if others could try it locally
For local development, I used yalc so I could test changes with the electron-quick-start app
Env setup from scratch (line by line for an easy copy-paste)
And now the magical brute-force one-liner for whenever you make changes! Rebuilds electron-builder, patches the npm module in electron-quick-start, and begins packaging.
If wanting to use a package.json, this is the config:
If using native addons, it's likely they haven't supplied compiled assets yet for darwin-arm64, so you'll need to rebuild from source. That also works with this PR, just add
"buildDependenciesFromSource": true
and presto, arm64 native addon.Example output (with spacing for better legibility) if using sqlite3 and electron-quick-start app with the env described above: