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

Electron universal builds for macOS only include the .dylib file for a single architecture #3622

Closed
3 tasks done
Nantris opened this issue Apr 11, 2023 · 20 comments
Closed
3 tasks done
Labels
Milestone

Comments

@Nantris
Copy link

Nantris commented Apr 11, 2023

Possible bug

It seems like the .node binaries for x64 and ARM64 are bundled in our app, but the .dylib file is only for x64. Is that expected? I'm guessing not.

Relevant comment on another issue: electron-userland/electron-builder#7512 (comment)

Is this a possible bug in a feature of sharp, unrelated to installation?

My first thought was to say "no, this is an installation-related bug, I believe" - but now I'm not clear one where this falls.

  • Running npm install sharp completes without error.
  • Running node -e "require('sharp')" completes without error.

If you cannot confirm both of these, please open an installation issue instead.

Are you using the latest version of sharp?

  • I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.

If you cannot confirm this, please upgrade to the latest version and try again before opening an issue.

If you are using another package which depends on a version of sharp that is not the latest, please open an issue against that package instead.

What is the output of running npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp?

System:
    OS: macOS 12.6.4
    CPU: (4) x64 Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
    Memory: 1.44 GB / 8.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.18.0 - ~/.nvm/versions/node/v16.18.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.18.0/bin/yarn
    npm: 8.19.2 - ~/.nvm/versions/node/v16.18.0/bin/npm
    Watchman: 2023.01.23.00 - /usr/local/bin/watchman
  npmPackages:
    sharp: ~0.32.0 => 0.32.0

What are the steps to reproduce?

Clone this repo (specifically the sharpIssue branch) and run yarn - this will install and build with electron-builder. You can see the .dylib file is only available for the x64 version as in my screenshot (or presumably only the ARM64 version if running on a Mac ARM64 machine.)

What is the expected behaviour?

Universal builds have access to the necessary assets for running on either x64 or ARM64

I'm unsure if this is a Sharp issue or an @electron/universal issue.

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem

Clone this repo - see instructions above

Please provide sample image(s) that help explain this problem

image

@Nantris Nantris added the triage label Apr 11, 2023
@lovell lovell added question and removed triage labels Apr 11, 2023
@lovell
Copy link
Owner

lovell commented Apr 11, 2023

sharp does not provide universal binaries for macOS.

Does the following change to electron-builder.js do what you need?

- arch: 'universal',
+ arch: ['x64', 'arm64'],

In summary, you need to get it to run the same commands as the final example in https://sharp.pixelplumbing.com/install#cross-platform

I note you commented on electron-userland/electron-builder#5689 where this has been discussed previously, and which may still be relevant.

@Nantris
Copy link
Author

Nantris commented Apr 11, 2023

Thanks for your reply @lovell. If I understand correctly the only option we have is to build an entirely separate installer for ARM64 - but our build process doesn't depend on universal binaries per se, so I still feel confused about why (and whether) this truly isn't possible.

I don't know how familiar you are with the workings of @electron/universal and I don't know how many mistaken assumptions I may have about the process, but this is my understanding:

Rather than using universal binaries, @electron/universal packages both architectures' individual binaries into a single installer. So my thinking is that it should be possible to use it in an Electron universal app still because @electron/universal if the libvips folder for ARM64 is copied into the installer. Right now you can see it copies both .node binaries, but only the libvips binary for a single architecture.

From their README:

The app is twice as big now, why?
Well, a Universal app isn't anything magical. It is literally the x64 app and the arm64 app glued together into a single application. It's twice as big because it contains two apps in one.


So, since prebuilt libvips binaries are available for both architectures, it should be possible to include both architectures in the app with that approach, no? The sharp-darwin-x64.node file expects libvips in vendor/8.14.2/darwin-x64 and the sharp-darwin-arm64v8.node expects libvips in vendor/8.14.2/darwin-arm64v8. Or is it more complicated than that? My thinking after your reply is that this is a shortcoming in @electron/universal and not a hard blocker within Sharp itself.

If simply getting the ARM64 libvips binary bundled into the proper location would fix this, then I'm thinking I need to continue this with @electron/universal's team. Or is there really a hard block within Sharp that prevents this workaround they use from working - and it would fail even if we copied the correct binaries to the correct location? If there is a hard blocker to this approach, what is the cause? Is there some code that is hardcoded to look for a certain architecture?

@Nantris Nantris changed the title Electron universal builds for macOS only include the .dylib file for a single architecutre Electron universal builds for macOS only include the .dylib file for a single architecture Apr 12, 2023
@lovell
Copy link
Owner

lovell commented Apr 12, 2023

Ah I see, so "universal" in Electron parlance doesn't refer to a single "fat" dylib for multiple architectures.

sharp supports multiple architectures and platforms in the same installation tree so in theory I guess it can work, but I don't know enough about the internals of electron-builder/universal to say what's missing here.

@lovell
Copy link
Owner

lovell commented May 11, 2023

@slapbox Were you able to make any progress with this?

@Nantris
Copy link
Author

Nantris commented May 11, 2023

Thanks for the follow-up @lovell! I see I forgot to link the issue I created here. I think if this issue is addressed in @electron/universal then we'll be in business: electron/universal#67

So I think there's nothing to do on the Sharp side for now.

@mushan0x0
Copy link

@slapbox Thank you for your tireless efforts on this issue. Currently, I also encountered this problem and tried many methods but none of them worked.

@MarshallOfSound
Copy link

MarshallOfSound commented May 12, 2023

Ah I see, so "universal" in Electron parlance doesn't refer to a single "fat" dylib for multiple architectures.

It does, @electron/universal just runs lipo under the hood to merge both architectures into a single file. It does some electron specific magic for our ASAR files but dylibs / frameworks / executables are dealt with the same as any other macOS universal app

Ref: https://github.com/electron/universal/blob/main/src/index.ts#L163-L169

@Nantris
Copy link
Author

Nantris commented May 15, 2023

@mmaietta and @lovell - thank you for your patience with all of my posts while I worked through this issue on multiple fronts. I wonder if you have any thoughts on how to approach a final resolution for this issue in light of this comment from @MarshallOfSound which outlines the problem - copied below:

@slapbox I had a chance to look into this, it looks like this is super specific to sharp in that the native module build process does not generate that dylib, instead it's generated at install time by the install/libvips script.

Although @electron/rebuild will correctly rebuild the native module it has no idea about the existence of that dylib build process. You'll have to somehow tell electron-builder (no idea how, we don't support that tool) to run the libvips script for each arch during the package phase so that each version of the packaged app has the right dylib.

Closing this out as it's a build script issue not an issue with the universal library

@mmaietta
Copy link

@slapbox please try this patch-package with buildDependenciesFromSource = true in your electron-config. This should add npm install to the build process flow via forceInstall, which is what I understand needs to be done?
app-builder-lib+24.4.0.patch

diff --git a/node_modules/app-builder-lib/out/packager.js b/node_modules/app-builder-lib/out/packager.js
index 35bffa6..0520db8 100644
--- a/node_modules/app-builder-lib/out/packager.js
+++ b/node_modules/app-builder-lib/out/packager.js
@@ -423,7 +423,8 @@ class Packager {
                 return;
             }
         }
-        if (config.buildDependenciesFromSource === true && platform.nodeName !== process.platform) {
+        const forceInstall = config.buildDependenciesFromSource === true;
+        if (forceInstall && platform.nodeName !== process.platform) {
             builder_util_1.log.info({ reason: "platform is different and buildDependenciesFromSource is set to true" }, "skipped dependencies rebuild");
         }
         else {
@@ -431,7 +432,7 @@ class Packager {
                 frameworkInfo,
                 platform: platform.nodeName,
                 arch: builder_util_1.Arch[arch],
-            });
+            }, forceInstall);
         }
     }
     async afterPack(context) {

@lovell
Copy link
Owner

lovell commented May 16, 2023

@slapbox If you're going down the universal binary route, have you tried building your own libvips dylib via lipo, as Samuel suggests Electron is already doing with the sharp.node file?

npm install --platform=darwin --arch=x64 sharp
npm rebuild --platform=darwin --arch=arm64 sharp
lipo node_modules/sharp/vendor/8.14.2/darwin-x64/lib/libvips-cpp.dylib.42 node_modules/sharp/vendor/8.14.2/darwin-arm64/lib/libvips-cpp.dylib.42 -create -output ...

You might also need to verify or set rpath on these universal binaries using install_name_tool.

Otherwise I'd stick to shipping multiple single-architecture files and let sharp (continue to) select the most relevant at runtime.

@Nantris
Copy link
Author

Nantris commented May 17, 2023

Thanks so much to both of you for your help in addition to all of the great works you've already shared; I appreciate it a lot!

@lovell thanks for the idea! I'll keep that in mind, but I'm hoping to avoid it as I have only a superficial knowledge of the required tools and ideas and it seems like we're closer than I'd thought to having this working thanks for your suggestion about npm rebuild.

@mmaietta I tried your patch but it seems that I'm unable to get Sharp to rebuild from source with or without the patch - perhaps due to not having the sharp/vendor/8.14.2/darwin-x64/includes/vips directory, but that's just a guess.

Note that this image is the state of the sharp directory after running the npm rebuild --platform=darwin --arch=arm64 sharp command advised by @lovell.

If I run electron-builder after running that command, that the resulting .app file contains both of the required .dylib files in their correct directories. I believe this .app file would work on both platforms, but I have no ARM64 machine to test with. It at least ran properly on my x64 machine and contains the expected files. Perhaps there's some way to leverage that approach

image

@mmaietta this is the output of the build command when enabling buildDependenciesFromSource without the patch.

Without patch TOUCH Release/obj.target/libvips-cpp.stamp CC(target) Release/obj.target/nothing/node_modules/node-addon-api/nothing.o LIBTOOL-STATIC Release/nothing.a warning: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: archive library: Release/nothing.a the table of contents is empty (no object file members in the library define global symbols) CXX(target) Release/obj.target/sharp-darwin-x64/src/common.o ../src/common.cc:13:10: fatal error: 'vips/vips8' file not found #include ^~~~~~~~~~~~ 1 error generated. make: *** [Release/obj.target/sharp-darwin-x64/src/common.o] Error 1 Error: `make` failed with exit code: 2 at ChildProcess.onExit (/Users/me/Documents/project/node_modules/node-gyp/lib/build.js:203:23) at ChildProcess.emit (node:events:513:28) at ChildProcess._handle.onexit (node:internal/child_process:291:12)

⨯ node-gyp failed to rebuild '/Users/me/Documents/project/node_modules/sharp' failedTask=build stackTrace=Error: node-gyp failed to rebuild '/Users/me/Documents/project/node_modules/sharp'
at ChildProcess. (/Users/me/Documents/project/node_modules/@electron/rebuild/src/module-type/node-gyp/node-gyp.ts:131:16)
at ChildProcess.emit (node:events:513:28)
at Process.ChildProcess._handle.onexit (node:internal/child_process:291:12)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

With the patch it fails with a more interesting error that I've never seen before where a monorepo package is causing the failure due to (apparently) not being able to find expo-yarn-workspaces to run the postinstall

With patch • installing production dependencies platform=darwin arch=x64 appDir=/Users/me/Documents/project ⨯ /Users/me/.nvm/versions/node/v18.16.0/bin/node process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE Exit code: 127 Output: [1/4] Resolving packages... [2/4] Fetching packages... [3/4] Linking dependencies... [4/4] Building fresh packages... info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Error output:
...standard yarn install warnings removed for space...

error /Users/me/Documents/project/node_modules/project-mobile: Command failed.
Exit code: 127
Command: expo-yarn-workspaces postinstall
Arguments:
Directory: /Users/me/Documents/project/node_modules/project-mobile
Output:
/bin/sh: expo-yarn-workspaces: command not found
failedTask=build stackTrace=Error: /Users/me/.nvm/versions/node/v18.16.0/bin/node process failed ERR_ELECTRON_BUILDER_CANNOT_EXECUTE
Exit code:
127
Output:
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
Error output:
...standard yarn install warnings removed for space...

error /Users/me/Documents/project/node_modules/project-mobile: Command failed.
Exit code: 127
Command: expo-yarn-workspaces postinstall
Arguments:
Directory: /Users/me/Documents/project/node_modules/project-mobile
Output:
/bin/sh: expo-yarn-workspaces: command not found
at ChildProcess. (/Users/me/Documents/project/node_modules/builder-util/out/util.js:235:20)
at Object.onceWrapper (node:events:628:26)
at ChildProcess.emit (node:events:513:28)
at maybeClose (node:internal/child_process:1091:16)
at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Lastly here's the output if I enable buildDependenciesFromSource AND nodeGypRebuild without the patch:

Without patch - `buildDependenciesFromSource` AND `nodeGypRebuild` set to `true` • executing @electron/rebuild arch=x64 version=22.3.6 appDir=/Users/me/Documents/project • packaging platform=darwin arch=x64 electron=22.3.6 appOutDir=release/dev/mac-universal-x64-temp • executing @electron/rebuild arch=arm64 version=22.3.6 appDir=/Users/me/Documents/project • executing @electron/rebuild arch=arm64 version=22.3.6 appDir=/Users/me/Documents/project • packaging platform=darwin arch=arm64 electron=22.3.6 appOutDir=release/dev/mac-universal-arm64-temp • packaging platform=darwin arch=universal electron=22.3.6 appOutDir=release/dev/mac-universal ⨯ Detected unique file "node_modules/sharp/build/Release/sharp-darwin-arm64v8.node" in "/Users/me/Documents/project/release/dev/mac-universal-arm64-temp/project-Dev.app/Contents/Resources/app.asar" not covered by allowList rule: "undefined" failedTask=build stackTrace=Error: Detected unique file "node_modules/sharp/build/Release/sharp-darwin-arm64v8.node" in "/Users/me/Documents/project/release/dev/mac-universal-arm64-temp/project-Dev.app/Contents/Resources/app.asar" not covered by allowList rule: "undefined" at checkSingleArch (/Users/me/Documents/project/node_modules/@electron/universal/src/asar-utils.ts:69:11) at Object.exports.mergeASARs (/Users/me/Documents/project/node_modules/@electron/universal/src/asar-utils.ts:123:7) at exports.makeUniversalApp (/Users/me/Documents/project/node_modules/@electron/universal/src/index.ts:231:13) at MacPackager.doPack (/Users/me/Documents/project/node_modules/app-builder-lib/src/macPackager.ts:129:9) at MacPackager.pack (/Users/me/Documents/project/node_modules/app-builder-lib/src/macPackager.ts:196:7) at Packager.doBuild (/Users/me/Documents/project/node_modules/app-builder-lib/src/packager.ts:442:9) at executeFinally (/Users/me/Documents/project/node_modules/builder-util/src/promise.ts:12:14) at Packager._build (/Users/me/Documents/project/node_modules/app-builder-lib/src/packager.ts:376:31) at Packager.build (/Users/me/Documents/project/node_modules/app-builder-lib/src/packager.ts:337:12) at executeFinally (/Users/me/Documents/project/node_modules/builder-util/src/promise.ts:12:14) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@Nantris
Copy link
Author

Nantris commented May 31, 2023

@mmaietta I'm so sorry to be such a pain with this issue.

I wonder if you had any other thoughts about possible resolution via electron-builder?

It seems like we can use the workaround of npm rebuild --platform=darwin --arch=arm64 sharp before we package, but I've not been able to test that personally since I don't have the relevant machine, and I'd love to see this handled by Electron Builder if possible so it would be less brittle and more universally accessible.

Can I help move to move us forward on this? If so, just let me know how.

I'd be happy to prepare another installer with this method for you to try to validate the fix, if it might be possible to leverage the workaround. Or to make a repro of Electron with Sharp so you can try the workaround yourself. I'd assume npm rebuild --platform=darwin --arch=x64 sharp should work on your end the same as the arm64 rebuild did for me?

@mmaietta
Copy link

mmaietta commented Jun 7, 2023

Hey @slapbox ! Not a pain at all 🙂
I haven't had any time as of late (startup life 😅 ) to keep up with electron-builder.

Can you put together a small sample gist/repo that I can fork/look into this and replicate?
I have an arm64 mac I can test with, but the env should be identical to x64 AFAICT.

@Nantris
Copy link
Author

Nantris commented Aug 2, 2023

@mmaietta I apologize for taking so long to get around to this. Startup life indeed!

I've put together a very basic repro here that runs the relevant npm rebuild commands in the postinstall step, based on a repro you previously provided me. I confirmed that the bundled app.asar includes the folders for both architectures under node_modules/sharp/vendor

Testing whether this fix works on your ARM64 should be as simple as:

  1. Clone repro repo
  2. npm install && npm run build && npm run app:pack
  3. Run the packaged app

No errors upon running suggests success. The expected visual output is something like this:

image


@lovell are the .h files in the includes folder required at application runtime for arm64? I assume so, but I've never needed to include header files before in a bundled application, so maybe not?

image

@lovell
Copy link
Owner

lovell commented Aug 3, 2023

@slapbox the vendor/.../include directory will be used at npm install time only when compiling sharp source against a prebuilt libvips. It is not used at runtime.

@mmaietta
Copy link

mmaietta commented Aug 4, 2023

@slapbox confirming it works.

Also, electron-builder had to migrate back to using the golang library for native modules as it has prebuild-install directly integrated in, latest version is now 24.6.3

@Nantris
Copy link
Author

Nantris commented Aug 5, 2023

Thanks for testing @mmaietta! Did you happen to test using the latest version of Electron-Builder? I saw that the new version existed but I didn't want to throw any extra variables into the mix at that point.

@lovell
Copy link
Owner

lovell commented Nov 22, 2023

I'm hoping that most of this problem will go away with the forthcoming sharp v0.33.0 and that anyone building electron apps can simply add the platforms they wish to support as explicit dependencies or devDependencies of their app, e.g add @img/sharp-darwin-arm64 and @img/sharp-darwin-x64 for "universal" macOS support.

There's a release candidate available should anyone wish to test - details at #3750 (comment)

@lovell lovell added this to the v0.33.0 milestone Nov 22, 2023
@Nantris
Copy link
Author

Nantris commented Nov 22, 2023

Thank you so much for your great work @lovell!

@lovell
Copy link
Owner

lovell commented Nov 29, 2023

v0.33.0 is now available.

@lovell lovell closed this as completed Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants