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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect npm_config_arch for cross-compilation scenarios #567

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dennisameling
Copy link

@dennisameling dennisameling commented Feb 22, 2021

Electron supports cross-compilation (e.g. from x64 to arm64) through the npm_config_arch environment variable: https://www.electronjs.org/docs/tutorial/using-native-node-modules#using-npm

prebuild-install checks the target architecture in a three-step process:

  1. pkgConf.arch (through running prebuild install --arch XXX)
  2. process.env.npm_config_arch for cross-compilation scenarios (e.g. a x64 host targeting arm64)
  3. process.arch for the architecture that Node is currently running on

This PR also brings that behavior to node-pre-gyp 馃殌

@springmeyer
Copy link
Contributor

@dennisameling thanks. A few questions:

  • Is this a feature enhancement or a regression fix?
  • This will need a test to ensure it does not regress before merging. Is that something you can work on adding to this PR?

@dennisameling
Copy link
Author

  • I guess a feature enhancement. I was just working on a repo which uses node-pre-gyp and noticed that it uses ARM64 in the build process, but the generated binary ended in x64. So I guess it has never worked in scenarios that use npm_config_arch
  • Sure, will add the test later today or tomorrow 馃憤馃徏

@springmeyer
Copy link
Contributor

Sure, will add the test later today or tomorrow

馃憤 thanks.

@dennisameling
Copy link
Author

Just a quick note that I didn't get to writing the test today - have rescheduled for tomorrow. Will keep you posted, sorry for the delay

@dennisameling
Copy link
Author

dennisameling commented Feb 24, 2021

@springmeyer just added the test. The weird thing is if that I run tape test/*test.js the test passes, but if I run npm run test it doesn't (am on Windows). Looks like stderr is empty in the latter case. Do you have any idea why that could be? Thanks in advance!

Also, Linux doesn't support cross-compilation unless things like crossbuild-essential-${ARCH} are installed. It does certainly work on Windows MSVC and I think on MacOS as well. Shall I only run the tests on Windows for now?

@springmeyer
Copy link
Contributor

springmeyer commented Feb 25, 2021

@springmeyer just added the test. The weird thing is if that I run tape test/*test.js the test passes, but if I run npm run test it doesn't (am on Windows). Looks like stderr is empty in the latter case. Do you have any idea why that could be? Thanks in advance!

Bizarre, no. Are all your tests uniquely named?

Also, Linux doesn't support cross-compilation unless things like crossbuild-essential-${ARCH} are installed. It does certainly work on Windows MSVC and I think on MacOS as well. Shall I only run the tests on Windows for now?

Right. Hmm. Yeah, would be nice to test this without compiling. Perhaps you could just have a test that runs node-pre-gyp install --loglevel=http and while that would fail you could assert on the output of path and that it contains the right target platform.

Or instead add a test that looks at the versioning specifically without messing with a build or install: https://github.com/mapbox/node-pre-gyp/blob/master/test/versioning.test.js

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.

None yet

2 participants