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

Binaries via optional dependencies doesn't get correct arch for Electron #376

Open
timfish opened this issue Apr 6, 2021 · 12 comments
Open
Assignees

Comments

@timfish
Copy link
Sponsor

timfish commented Apr 6, 2021

When using electron-forge or electron-builder we generally build our Windows apps for both ia32 and x64.

On the surface, the optional dependencies route seems like a good idea but it doesn't work in the above case because npm optional modules are installed for the current nodejs architecture, not for the target Electron arch.

None of the Electron tooling appears aware of binaries via optional dependencies route so it doesn't work for one of the architectures.

@timfish
Copy link
Sponsor Author

timfish commented Apr 6, 2021

My total build assets come to under 3MB so I'm going to try shipping all assets in a single package and copy to {package_name}.node from the install script. This also negates the need to use the helper at runtime and therefore fixes #316.

@Brooooooklyn Brooooooklyn self-assigned this Apr 7, 2021
@timfish
Copy link
Sponsor Author

timfish commented Apr 7, 2021

I've got everything in one package and copying across in an install script but this doesn't actually fix the issue with Electron.

It appears that the reason this all "just works" for modules built with node-gyp is because electron-rebuild/electron-forge/electron-builder all have special cases for node-gyp. 🤔

I had assumed it would be rebuilding via npm rebuild...

@Brooooooklyn
Copy link
Sponsor Member

On the surface, the optional dependencies route seems like a good idea but it doesn't work in the above case because npm optional modules are installed for the current nodejs architecture, not for the target Electron arch.

Is yarn install --ignore-platform a temporary solution?

@timfish
Copy link
Sponsor Author

timfish commented Apr 10, 2021

That might work but with my workaround for #316, webpack will pickup all the .node files and include them in the output.

If I don't bundle, node_modules will have have all the binaries too.

There's talk of improving electron-rebuild to cater for other build systems. There's demand for this from neon too!

@timfish
Copy link
Sponsor Author

timfish commented Jun 15, 2021

My currently workaround is:
install.js called on install

const { copyFileSync, existsSync } = require('fs');
const { platform, arch } = require('os');
const { platformArchTriples } = require('@napi-rs/triples');
const { join } = require('path');

const platform = process.env.npm_config_target_platform || platform();
const arch = process.env.npm_config_target_arch || arch();

const triples = platformArchTriples[platform][arch];
const tripe = triples[0];
const SRC_FILE = `my-module.${tripe.platformArchABI}.node`;
const DST_FILE = './my-module.node';

const publishSrc = join('./artifacts', SRC_FILE);
if (existsSync(publishSrc)) {
  copyFileSync(publishSrc, DST_FILE);
  return;
}

const devSrc = join('./', SRC_FILE);
if (existsSync(devSrc)) {
  copyFileSync(devSrc, DST_FILE);
  return;
}

And index.js is nice and simple:

const binding = require('./my-module.node');

This way I can call npm rebuild --target-arch=x64 --target-platform=linux and the correct binary gets copied.

@nickfujita
Copy link

Hello, stumbled upon this issue when trying to investigate and electron build issue my team is having after switching over from adm-zip to yauzl-promise, which uses @node-rs/crc32. The problem we are running into is when using electron-builder on a macos x64 system to perform output builds of an electron app for both x64 and arm64. Curiously, even when electron-builder is packaging for arch arm64, it still includes the x64 version of crc32 for some reason.

@timfish does this sound like the same issue you were seeing, and if so, have you found another solution other than the workaround you posted back in 2021? 🙏🏼

@timfish
Copy link
Sponsor Author

timfish commented Aug 7, 2023

I released a modiule that can find/delete imcompatible binaries:
https://github.com/timfish/incompatible-binaries

You can use this with Electron Builder using the afterPack hook:

const { deleteIncompatibleBinaries } = require('incompatible-binaries');

const ARCHES = ['ia32', 'x64', 'arm', 'arm64', 'universal'];

exports.default = function (context) {
  const arch = ARCHES[context.arch];

  console.log('Deleting incompatible binaries', context.appOutDir, process.platform, arch);

  deleteIncompatibleBinaries(context.appOutDir, process.platform, arch);
}

@nickfujita
Copy link

@timfish Thank for the module 👍🏻 So it sounds like you are manually loading the modules for all architectures at build time, then using this script to delete the ones that it doesn't need?

The issue we are seeing is that when using electron-builder tries to build for both x64 and arm64 it will ONLY pick up the optional dependency for the architecture of the machine performing the build, rather than the architecture specified by electron-builder. So in this case, using github actions macos-latest image, it only picks up the x64 optional dependency, even though electron-builder specifies it's building arm64. Just want to reconfirm if that was the original issue you were seeing as well here, but for Windows ia32 and x64 architectures?

@Brooooooklyn
Copy link
Sponsor Member

Let's wait for this: npm/rfcs#612

@timfish
Copy link
Sponsor Author

timfish commented Aug 8, 2023

The issue we are seeing is that when using electron-builder tries to build for both x64 and arm64 it will ONLY pick up the optional dependency for the architecture of the machine performing the build

The native modules I use all ship all binaries in a single package so they're all included by the bundlers and incompatible-binaries stips out the ones which aren't required

@nickfujita
Copy link

Thanks🙏🏼

Eventually got it working using 'yarn install --ignore-platform' plus the afterPack hook and deleteImcompatibleBinaries function 🎉

@yukukotani
Copy link

yukukotani commented Sep 16, 2023

@Brooooooklyn npm/rfcs#612 has been landed in npm v10.1.0 and will be backported to v9.9.0 as well.

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

No branches or pull requests

4 participants