Apple Silicon macOS test/builds #3077
Conversation
| @@ -69,7 +69,7 @@ runs: | |||
| env: | |||
| GH_TOKEN: ${{ inputs.gh_token }} | |||
| APPLE_ID: ${{ inputs.apple_id }} | |||
| APPLE_ID_PASSWORD: ${{ inputs.apple_id_password }} | |||
| APPLE_APP_SPECIFIC_PASSWORD: ${{ inputs.apple_id_password }} | |||
There was a problem hiding this comment.
As we transition from the separate electron-builder-notarize to leveraging the notarization support that's in electron-builder, the name of this environment variable changes but everything else stays the same.
| @@ -87,5 +87,5 @@ runs: | |||
| - name: Check notorization with gatekeeper | |||
| if: runner.os == 'macOS' | |||
| run: | | |||
| spctl --assess --type execute --verbose --ignore-cache --no-cache dist/apps/zui/mac/*.app | |||
| spctl --assess --type execute --verbose --ignore-cache --no-cache dist/apps/zui/mac*/*.app | |||
There was a problem hiding this comment.
The wildcard is needed here because the Apple Silicon builds end up in a subdirectory mac-arm64/ whereas the Intel ones were in mac/.
.github/actions/setup-zui/action.yml
Outdated
| - name: Install Node | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| cache: yarn | ||
| # cache: yarn |
There was a problem hiding this comment.
In an early test iteration, @mattnibs spotted that the zed binaries bundled with Zui were still showing up as Intel. I ultimately traced it to a side effect of this caching, then found open issue actions/setup-node#1008 that seems to indicate this is a known problem. I briefly went down the path of trying to find another way to keep caching in play via some other config, but I also noticed that disabling caching wasn't slowing down the build process much at all. In the interest of simplicity I figured we could just live without it for now. I'm Subscribed to that issue so if/when it gets addressed I could revisit this. In the meantime I've included this comment so someone doesn't turn it on again without recognizing the effect it'll have on the macOS builds.
apps/zui/electron-builder.json
Outdated
| @@ -10,11 +10,11 @@ | |||
| "sign": "./scripts/sign.js" | |||
| }, | |||
| "linux": {"target": ["deb", "rpm"]}, | |||
| "mac": {"entitlements": "darwin.plist", "notarize": {"teamId": "2DBXHXV7KJ"}}, | |||
There was a problem hiding this comment.
The teamId value here is redundant with the value in the APPLE_TEAM_ID environment variable, but it's needed here due to open issue electron-userland/electron-builder#8103. I'm Subscribed to that issue so I can revisit this if/when it gets fixed. Also, found consensus validated my guess that this team ID does not seem to be sensitive info (e.g., similar to a company/domain name), so I've gone ahead and put the value here rather than adding yet more code to populate it from a Secret.
| // both regular Zui and Zui Insiders. | ||
| const OWNER = pkg.repository.split('/')[3]; | ||
| const REPO = pkg.repository.split('/')[4]; | ||
| const PRODUCT_NAME= pkg.productName.replaceAll(' ', '-'); |
There was a problem hiding this comment.
This replaceAll was needed because the build filenames that start out with space characters in them (e.g., Zui - Insiders-1.7.1-19.dmg) have those spaces turned into hyphens by the time they're uploaded to GItHub. I briefly explored using this as an opportunity drop those space characters entirely, but post-update that ends up putting the new app state & saved data in a fresh, new directory ~/Library/Application Support/Zui-Insiders hence losing the prior state/data from the traditional ~/Library/Application Support/Zui - Insiders, hence that would require a new migration logic for users to have a seamless update. Yuck! Hence the replaceAll to just keep going with what we've got.
| const REPO = 'zui'; | ||
| const URL = `/repos/${OWNER}/${REPO}/releases`; | ||
| const VERSION = pkg.version; | ||
| const RELEASE_NAME = (PRODUCT_NAME == 'Zui') ? 'v' + VERSION : VERSION; |
There was a problem hiding this comment.
Here's another coping mechanism. For all the time they've existed, Zui Insiders builds have names without the leading v (e.g., 1.7.1-19) while regular Zui have it (e.g., v1.7.0). Here I rationalized leaving things as they are because GitHub already does some annoying things with how it sorts releases, so I didn't want to make things even worse by also changing the naming convention.
| try { | ||
| const originalAsset = currentRelease.assets.find(asset => { | ||
| return asset.name === FILE_NAME; | ||
| }) | ||
|
|
||
| if (!originalAsset) { | ||
| console.log(`[remote] ${FILE_NAME} not found. Skipping merge`); | ||
| return; | ||
| } else { | ||
| console.log(`[remote] ${FILE_NAME} found`) | ||
| } |
There was a problem hiding this comment.
I would move this stuff out of the try block since that it was you are doing elsewhere in this file. Also you can drop the else and just console log.
| "mac": { | ||
| "entitlements": "darwin.plist", | ||
| "notarize": {"teamId": "2DBXHXV7KJ"}, | ||
| "artifactName": "${productName}-${version}-${arch}.${ext}" |
There was a problem hiding this comment.
At first when using electron-builder at its defaults, we'd get a a DMG for Intel like Zui-1.7.4.dmg and for Apple Silicon like Zui-1.7.4-arm64.dmg. This use of artifactName gives us symmetry by having the Intel one now have the arch included as well and hence Zui-1.7.4-x64.dmg.
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
…via push Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Matthew Nibecker <[email protected]>
Co-authored-by: Noah Treuhaft <[email protected]>
tl;dr
Now that GitHub offers free Actions runners based on Apple Silicon, it becomes feasible for us to run CI and create builds for this platform. While getting this working, I managed to simplify the build process a little (was able to drop the separate
electron-builder-notarizedependency) but also had to add some stuff to work around rough edges in the tooling. Details are below and in-line PR comments.Actions Runners
Per GitHub docs, the Runner
macos-14is on Apple Silicon while themacos-12we've been using continues to run on Intel. While I don't anticipate platform-specific issues, I've gone ahead here and addedmacos-14to the OS matrix for our CI so we can continue to test on the same platforms we build for.Dropping
electron-builder-notarizeEarly in my research I could tell that there were still some lingering growing pains in the tooling related to Apple Silicon. Rather than burn time bumping into what might be known problems that were already fixed, my first step was therefore to advance to the newest release of
electron-builder. It's at this point I was reminded thatelectron-buildercan now do notarization on its own as an alternative to relying on the separateelectron-builder-notarizetool as we've done in the past. Indeed, I think I recall @jameskerr having once spotted this and we'd made a mental note to look into it, but I used "if it ain't broke" logic to justify putting it off. In this case I knew I was likely to break and re-fix our build system anyway, so I figured I'd bite the bullet and maybe spare myself from having to work around problems in yet another tool.This admittedly did send me briefly down a rabbit hole when I saw in the
electron-builderMac docs that they recommended a different set of credentials (based on API keys) over the password-based approach we'd been using withelectron-builder-notarize(explanation in electron-userland/electron-builder#7859). I spent a little time looking at what it would take to start using this alternate approach but ended up backing off because API keys seemed linked to "App Store Connect", hence the assumption we were building something to be published into the App Store, so first steps had Apple asking me to sign new agreements, entering details on payments/taxes related to all the revenue we'd surely make from our App Store sales, etc. Maybe it would have been possible to take these steps only as far as necessary to just get the API keys and not actually publish anything to the App Store. However, just as this was making me nervous, I found that I could extend the password-based approach we've been using all this time inelectron-builderby making two small changes:APPLE_ID_PASSWORDbecomesAPPLE_APP_SPECIFIC_PASSWORD).teamIdvalue in theelectron-builder.json(which is redundant with theAPPLE_TEAM_IDenv var, but needed to work around open issue Notarize failed with 24.13.3 after upgrade from 24.12.0 with app specific password electron-userland/electron-builder#8103)Entitlements, App Sandbox, and Hardened Runtime
My first attempts built successfully but then when @mattnibs would try to run them they'd fail with a crash like:
After doings some web searches to see if others were seeing a similar crash, I eventually found my way back to the
electron-builderMac docs that include this passing comment:Sure enough, once I learned how to make that change, the Apple Silicon builds no longer crashed. However, since the concept of "entitlements" was unfamiliar to me, I felt the need to go on a detour to learn a bit about them to understand what they've been set for on our Intel builds all this time, if we should be setting others, etc. The docs were a bit confusing in spots since most seem to be written as if to an audience that have already committed their whole lives to developing in the Apple universe and hence provide limited context. But I'll summarize what I came to understand, with the caveat that someone "more developer than me" could surely explain it better and/or find mistakes in my understanding.
This post taught me how to check an app's entitlements. For example, here's what our latest GA Zui Intel build v1.7.0 shows:
So, picking that apart, we see
com.apple.security.cs.allow-unsigned-executable-memoryandcom.apple.security.cs.disable-library-validation. We never took explicit steps to ask that these be included in our builds, but I found this file inelectron-builderthat references them, so that seems to explain why they're there by default. Strangely, that also shows an entry forcom.apple.security.cs.allow-jitand it's been in there since January, 2020 (electron-userland/electron-builder#4491) including in theelectron-builderversion I started using in this PR, so I'm still not 100% sure why I had to ask for it explicitly. But now that I was aware that the Apple docs effectively discourage the use of those two entitlements, I became more interested in knowing if we could live without them going forward.I did ultimately find evidence that we can leave them behind.
plistfile explains that thecom.apple.security.cs.allow-unsigned-executable-memoryentitlement is only needed for Electron 11, and that its use in Electron 12+ is discouraged due to the same security exposure cited by Apple.plistfile ultimately concludes with this comment that happens to be a fix I already put in Zui as part of Prevent packing of native modules #2917, indicating we can drop this entitlement as well.Finally, using the config in this PR that dropped those two entitlements and left us with only the JIT one, I did smoke testing with an Intel build doing all the typical user stuff (loading pcaps, opening slices in Wireshark, opening
whois, etc.) and it all went fine.It also should be noted that most mentions of entitlements are in the context of discussing App Sandbox, so I had to learn a bit about that as well. Consider the entitlements shown for a common app like Slack:
You'll notice entitlements in here like
com.apple.security.network.client. Zui has acted as a "network client" just fine all these years but we don't list this entitlement, so what's up? Well, what I've learned is how Apple offers this "App Sandbox" wrapper as a way to run apps more safely, and one requirement of this is that the app has to list all of its entitlements for the things it might potentially do, hence things like hitting the network, accessing your microphone, etc. To be distributed through the App Store, an app must run in Sandbox, so that explains why we saw what we did with Slack. Likewise, since Zui isn't distributed through App Store, we've never had to.If you're curious, you can add a column to Activity Monitor that'll show which apps are running in the Sandbox. While many are (e.g., Slack), many I use every day are not (e.g., Terminal, Google Chrome, Camtasia), so by no means is it a black eye that we're non-Sandbox.
To tie this all back to Entitlements, I ultimately recognized that dealing with them is necessary for Sandbox and being in the App Store, hence why I saw these topics mentioned together so often. However, there are also non-Sandbox/non-App-Store contexts when Entitlements manner, such as this JIT setting. But now we know a bit more should the day come that we want to start jumping through hoops to publish Zui in the App Store.
Yet another topic I encountered in my travels was Hardened Runtime, since that mentions how it disables JIT compilation and why the entitlement is therefore needed in our case (see also: Porting just-in-time compilers to Apple silicon for why it's needed with Apple Silicon but wasn't with Intel). An EntitlementCheck tool was ultimately useful to confirm that we've been using Hardened Runtime on our Zui builds all along, so once I confirmed we were in line with best practices there and the JIT entitlement got the Apple Silicon builds working, I was satisfied. Below is with the last GA Zui release v1.7.0:
Per its name, that same repo has a tool that flags undesirable entitlements, and indeed this also flagged the two that we'd had in the past and won't anymore with the new builds that use only the JIT entitlement:
Merging
latest-mac.ymlWhile the changes above got the Apple Silicon builds working, a problem remained with the auto-updates. The tl;dr is the update process relies on the file
latest-mac.ymlthat's generated during the build process. Here's an example of the one from the last GA Zui release v1.7.0:An Apple Silicon build would create a similar file but with entries for
Zui-1.7.4-arm64.dmgandZui-1.7.4-arm64-mac.zip. When running separate Intel and Apple Silicon builds converged on the same GitHub Releases destination like I'm doing in this PR, what ends up happening is that the build that finishes last pushes itslatest-mac.ymlto GitHub, overwriting the one generated by the first build.As mentioned in a comment in the
merge-mac-release-files.mjsscript added in this PR, I found an openelectron-builderissue where other users were struggling with the same problem and so I borrowed the code from this comment to create a workaround script to do the same for our build process. In brief, the builds for each macOS platform push their own separatelatestfiles temporarily to GitHub, then the build that finishes second downloads both files, merges them to a singlelatest-mac.ymlthat mentions both the Intel and Apple Silicon offerings, then pushes that file back up to GitHub and removes the temporary files. It's a little hacky, but it works.Upgrade Path
@mattnibs helped test all this using releases in personal forks I created of Zui and Zui Insiders where I could publish real releases without risk of impacting our existing user base. From what we found, here's what I expect we'll need to communicate to macOS users.
Having been running Intel builds all this time, if they continue to rely only on auto-update, they'll continue to be updated to Intel builds even as the Apple Silicon ones become available and referenced in the
latest-mac.yml.When a user wants to switch to Apple Silicon, they should do a one-time manual download/install of a
Zui-*-arm64.dmgbuild. During installation, they can accept the prompt to Replace the existing (Intel-based) Zui. All their settings and Zed lake data will remain intact.As newer releases come out, the users that did the one-time manual install of an Apple Silicon build will auto-update to the newer Apple Silicon builds.
Closes #1266