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

Apple M1 support #4140

Merged
merged 3 commits into from
Jan 12, 2022
Merged

Apple M1 support #4140

merged 3 commits into from
Jan 12, 2022

Conversation

johnwchadwick
Copy link
Contributor

@johnwchadwick johnwchadwick commented Oct 20, 2021

Overview

This PR enables Universal builds that have native support for Apple's M1 AArch64 processors.

Of interest for testing:

  • Check for regressions on AMD64 macOS, Windows, Linux
  • Make sure build process and tests are not impacted
  • Test on M1 Mac, of course :)

The body of this PR message will contain explanations and rationale for the "last mile" of changes necessary to enable builds for so-called Apple Silicon.

Explicitly use Kong/node-libcurl fork

One of the tricks originally used in this PR was to use the Kong/node-libcurl soft fork using a kind of alias, like so:

    "node-libcurl": "npm:@getinsomnia/node-libcurl@2.3.4-0",

This trick is very convenient, as it does not necessitate any other changes. You can still do require("node-libcurl"), from inside Webpack, outside Webpack, in a test, in the runtime environment, because as far as npm is concerned, the module is installed as node-libcurl.

This trick wound up being adopted for the Electron 12 upgrade, as the fork was needed for Electron 12.2 support. As such, it is currently in develop.

Unfortunately, for some reason, npm behaves differently when not using normal npm packages. One of the side-effects is that npm rebuild filters them out and thus they are not rebuilt. This is a problem because electron-builder, by a long chain of events through multiple dependencies, will call npm rebuild during the build of a macOS Universal build in order to rebuild native dependencies for a different architecture, but in this case npm rebuild will not run as expected, and the different binary will not be pulled.

As such, the alias has been removed and replaced with an explicit dependency on the fork, and rewrites and mocks have been rewritten accordingly.

Fix appUrl calculation

-  const url = process.env.APP_RENDER_URL;
-  const appUrl = url || `file://${app.getAppPath()}/renderer.html`;
+  const appPath = path.resolve(__dirname, './renderer.html');
+  const appUrl = process.env.APP_RENDER_URL || url.pathToFileURL(appPath).href;

In the past, the URL to load into the main window was calculated using electron.app.getAppPath(), which, in final builds, returns the main app.asar bundle.

However, Universal Electron builds are a bit different. When creating a universal build, first, a build is created for amd64, native npm modules are rebuilt for arm64, and then, a build is created for arm64. If these builds are identical, the asar bundles are merged into the normal app.asar.

The app.asar bundle can differ in the case where you have native modules for two different architectures, in which case you will get app-amd64.asar and app-arm64.asar with full copies of the app. Unfortunately, a stub app.asar exists, but it doesn't contain much, and getAppPath() will still return it, leading to failure. This makes it difficult to determine when the application should even expect architecture-specific asar bundles to exist.

The new solution works in any case and does not require detecting if architecture-specific bundles exist, simply by relying on the fact that renderer.html will always be next to the entrypoint script. We use URL.pathToFileURL from Node.JS 10 to ensure that our file URL is well-formed; this should prevent issues in cases where the path in which Insomnia is stored contains characters that may not appear unescaped inside a URI, a case that was previously not handled.

Other cases of getAppPath() are not subject to this issue, because they point outside of the bundle, so they are not currently touched. In the future, it might be a good idea to use process.resourcesPath instead for some of those cases.

Update node-libcurl fork.

Originally, I made the macOS build of node-libcurl output separate binaries for amd64 and arm64, concerned about how Electron Builder would behave if presented a universal binary. However, as it turns out, Electron Builder handles this case just fine: it will detect that the builds for both architectures are the same and use a single binary instead. Therefore, the fork is updated to output universal builds to both arm64 and amd64 targets for macOS. This greatly reduces the size of the Universal build on disk, as each app-[arch].asar bundle takes up approximately 150 MiB on disk, whereas the single app.asar bundle is about 160 MiB.

In addition to reducing the app size, it also prevents both of the above problems: the npm rebuild is no longer needed, and the split app.asar problem is gone. This should help ensure that further edge cases do not result from this change and hopefully will help if we ever need split bundles in the future (perhaps, for Windows some day?)

Enable universal builds.

Finally, a single-line change enables universal builds with electron-builder.

+    ...targetPlatform === 'mac' ? { universal: true } : {},

Fixes #2964.
Closes INS-1347
changelog(Improvements): Added support for Apple Silicon (Apple M1)

@johnwchadwick johnwchadwick marked this pull request as ready for review January 5, 2022 17:34
Copy link
Contributor

@jackkav jackkav left a comment

Choose a reason for hiding this comment

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

Nicely done!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Looks great and appears to be working well too, thank you for the clear description! Unfortunately I don't have access to an M1 mac, but it works on my 2019 MBP.

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

one little thing for you to weigh in on, then we're ready to merge. although I don't have an actual M1 to test on, I didn't have any troubles running locally on linux and the code looks correct.

johnwchadwick and others added 3 commits January 12, 2022 16:02
Co-Authored-By: John Chadwick <john.chadwick@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARM M1 support? :)
4 participants