mapbox / node-pre-gyp

Node.js tool for easy binary deployment of C++ addons
BSD 3-Clause "New" or "Revised" License
1.12k stars 264 forks source link

node-pre-gyp doesn't handle arm architectures properly. #348

Open nicolasnoble opened 6 years ago

nicolasnoble commented 6 years ago

The "arm" target in itself isn't enough. If you look at node binary distributions, you have 3 variants for arm: armv6l, armv7l, and arm64.

An example of an armv6l platform is a raspberry pi zero. Installing nodejs on a raspberry pi zero through nvm will yield the correct download of the correct binary (armv6l).

An example of an armv7l platform is a raspberry pi 3. Installing nodejs on a raspberry pi 3 through nvm will also yield the correct download of the correct binary (armv7l).

However, for both platforms, node-pre-gyp identify them as "arm", meaning we can only cross compile and publish a single version of node-pre-gyp packages, either armv6l or armv7l, which aren't interchangeable due to dynamic library versions and tags.

This creates a problem where users will be able to download the wrong version of the package for their platform, and crash on load. The only method for them to fix it is to uninstall the node-pre-gyp enabled package, and re-install it using a flag to force falling back on ignoring published packages and compile from source instead, which is not obvious.

Also, the documentation states that the only correct values for --target_arch are ia32, x64 and arm, whereas arm64 is in fact a valid keyword to use here. The produced packages work properly on an arm64 environment.

nicolasnoble commented 6 years ago

Note that testing these platforms requires only one raspberry pi zero, and one raspberry pi 3. The raspberry pi zero can then be used to have an armv6l architecture using the typical raspbian zero distribution, while the raspberry pi 3 can be used for both the armv7l architecture using the typical raspbian distribution, and the arm64 architecture using the pi64 distribution.

nicolasnoble commented 6 years ago

After digging the code a bit, I realized that the --target_arch argument isn't actually mapped to anything special, and that passing arm64 there should work properly. After experimenting a bit, it appears it does indeed work properly when cross compiling for arm64.

springmeyer commented 6 years ago

I realized that the --target_arch argument isn't actually mapped to anything special, and that passing arm64 there should work properly.

👍 correct. But it will default to ia32, x64, etc when these can be detected by using process.arch from node.js core.

nicolasnoble commented 6 years ago

I'm sorry, but that issue wasn't to be closed. While arm64 kinda works, armv6l still doesn't.

springmeyer commented 6 years ago

sorry, re-opening

nicolasnoble commented 6 years ago

To be more specific, while nodejs ships in two flavors of arm 32 bits, armv6l and armv7l (see the nodejs distribution files), node-pre-gyp makes no distinction between the two, and will attempt to download a prebuilt package named "arm" for both.

The problem really lies with how node-pre-gyp gets its architecture name. process.arch isn't enough, it needs to be mixed in with process.config.variables.arm_version in addition to it.

So the proper code should be something along these lines:

let arch = process.arch
if (arch === 'arm') {
  arch = 'armv' + process.config.variables.arm_version + 'l'
}
markandrus commented 5 years ago

I just ran into this issue, as I'm looking to publish ARM binaries for one of my projects. Would the change proposed by @nicolasnoble be acceptable?

mpowell90 commented 5 years ago

node-pre-gyp seems to be ignoring the --target_arch flag all together: `[root@buildroot server]# ./node_modules/.bin/node-pre-gyp install --directory=./node_modules/sqlite3/ --target_arch=arm --target_platform=linux --target=8.14.0 --update-binary --build-from-source

node-pre-gyp info it worked if it ends with ok

node-pre-gyp info using node-pre-gyp@0.10.3

node-pre-gyp info using node@8.12.0 | linux | x64

node-pre-gyp info chdir ./node_modules/sqlite3/

node-pre-gyp WARN Using request for node-pre-gyp https download

node-pre-gyp info build requesting source compile

gyp info it worked if it ends with ok

gyp info using node-gyp@3.8.0

gyp info using node@8.12.0 | linux | x64

gyp info ok ` Any suggestions on how to update binaries for armv7?

jupp0r commented 3 years ago

This is a problem for supporting modules built for apple silicon. The reliance on process.platform is problematic, because the host and target compiler platforms can be different (cross-compiling for arm64 on x64 MacOS is common).

springmeyer commented 3 years ago

@jupp0r I don't have access to one of these machines. Do you? If so can you provide more details?

inukshuk commented 3 years ago

@springmeyer yes, process.arch is arm64 on the ARM MacBooks. If you try loading x64 addons you get dlopen errors like:

no suitable image found.  Did find:
    [...]/node_modules/sqlite3/lib/binding/napi-v6-darwin-arm64/node_sqlite3.node: mach-o, but wrong architecture

That being said, with node-gyp 7.1.2 and node-pre-gyp 0.17.0 cross-compiling arm64 on x64 works for me. I'm passing both --arch and --target_arch so at least one of those seems to be picked up. Note that I was rebuilding sqlite3 and that only works by patching the module to use the latest node-gyp and node-pre-gyp listed above.

@jupp0r for cross-compiling arm64 I also had to set SDKROOT to the latests SDK (i.e., $(xcrun -sdk macosx --show-sdk-path)) since my default SDK did not know about arm64.

springmeyer commented 3 years ago

@inukshuk thanks. If process.arch is arm64 on silicon macbooks then I don't see a problem here with node-pre-gyp because the existing versioning information is suitable for macbooks. So the problem is that the module that integrates node-pre-gyp needs to properly create arm64 binaries, detect them, and require them at runtime. The error you are seeing:

/node_modules/sqlite3/lib/binding/napi-v6-darwin-arm64/node_sqlite3.node: mach-o, but wrong architecture

Indicates that the sqlite3 module is incorrectly packaging its binary. So this is a problem with node-sqlite3 then not node-pre-gyp.

szh commented 3 years ago

To be more specific, while nodejs ships in two flavors of arm 32 bits, armv6l and armv7l (see the nodejs distribution files), node-pre-gyp makes no distinction between the two, and will attempt to download a prebuilt package named "arm" for both.

The problem really lies with how node-pre-gyp gets its architecture name. process.arch isn't enough, it needs to be mixed in with process.config.variables.arm_version in addition to it.

So the proper code should be something along these lines:

let arch = process.arch
if (arch === 'arm') {
  arch = 'armv' + process.config.variables.arm_version + 'l'
}

Thanks for this! I added a PR with this change.

daniellockyer commented 2 years ago

Unfortunately I just ran into this when reworking node-sqlite3's prebuilt binaries 😕 It'd be great to fix armv6 and armv7 in node-pre-gyp but I'll have to keep them disabled for now