fix: instruct prebuild-install to download napi builds #47
Conversation
…es which declare themselves to use napi
|
Can confirm this PR works when testing with Before: After: |
| bin, | ||
| "--platform=" + configuration.Platform, | ||
| "--arch=" + configuration.Arch, | ||
| "--target=" + strconv.FormatUint(uint64(dependency.NapiVersions[0]), 10), |
There was a problem hiding this comment.
Do you really want to just take the first N-API version that it finds here? Isn't that a bit tricky? What if a native module supports multiple versions, like v3 and v4? I'm actually curious how prebuild-install finds which version to download, will check later
There was a problem hiding this comment.
You're right it probably should do something more intelligent, but because of abi stability this will produce something that is guaranteed to work while maybe not optimal, assuming the first is the lowest number.
This is definitely something that can be improved, but wasn't necessary for me to get something that worked (and none of the packages I use target multiple napi versions)
There was a problem hiding this comment.
Bingo! https://github.com/inspiredware/napi-build-utils/blob/master/index.js#L166-L179
This is used by prebuild-install. They look at process.versions.napi and select the highest available N-API version that's compatible with the currently running NodeJS version. Will do a quick check if we can implement something similar here as well :)
There was a problem hiding this comment.
Good news! It's not necessary to provide a target, because prebuild-install will automatically determine the most appropriate N-API version if the given runtime is N-API 🎉: https://github.com/prebuild/prebuild-install/blob/master/rc.js#L48-L50
After removing the --target, it indeed automatically finds the right N-API version:
• install prebuilt binary name=sharp version=0.27.1 platform=win32 arch=x64 napi=
• execute command command='C:\Program Files\nodejs\node.exe' 'D:\repos\electron-webpack-quick-start\node_modules\prebuild-install\bin.js' --platform=win32 --arch=x64 --runtime=napi
--verbose --force
workingDirectory=D:\repos\electron-webpack-quick-start\node_modules\sharp
• command executed executable=C:\Program Files\nodejs\node.exe
errorOut=prebuild-install info begin Prebuild-install version 6.0.1
prebuild-install info looking for cached prebuild @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
prebuild-install http request GET https://github.com/lovell/sharp/releases/download/v0.27.1/sharp-v0.27.1-napi-v3-win32-x64.tar.gz
prebuild-install http 200 https://github.com/lovell/sharp/releases/download/v0.27.1/sharp-v0.27.1-napi-v3-win32-x64.tar.gz
prebuild-install info downloading to @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz.25192-369cb427469f5.tmp
prebuild-install info renaming to @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
prebuild-install info unpacking @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a790f7-sharp-v0.27.1-napi-v3-win32-x64.tar.gz
prebuild-install info unpack resolved to D:\repos\electron-webpack-quick-start\node_modules\sharp\build\Release\sharp.node
prebuild-install info install Successfully installed prebuilt binary!
• all native deps were installed using prebuild-install
• exited command=app-builder.exe code=0 pid=29008
There was a problem hiding this comment.
| "--target=" + strconv.FormatUint(uint64(dependency.NapiVersions[0]), 10), |
There was a problem hiding this comment.
What runtime will it be using though?
For example if targeting a version of electron based around node 14, and you are running node 10 will that produce the correct result? I would expect it to be run in your node 10, and so get it wrong
There was a problem hiding this comment.
I just tested this hypothesis by testing this PR without the target being passed to prebuild-install.
I tested with native binary @dennisameling/keytar-temp@7.4.100-beta which has N-API prebuilds for N-API v3, v6 and v7 on Windows x64.
Test: Electron 11, NodeJS 14.15.5
Electron 11.2.3
process.versions.napi = 6
Node 14.15.5
process.versions.napi = 7
prebuild-install now tries to install a binary for N-API version 11.2.3, which is obviously incorrect as it's the Electron version.
• install prebuilt binary name=@dennisameling/keytar-temp version=7.4.100-beta platform=win32 arch=x64 napi= �
• execute command command='C:\Program Files\nodejs\node.exe' 'D:\repos\my-electron-app\node_modules\prebuild-install\bin.js' --platform=win32 --arch=x64 --runtime=napi --verbose --force
workingDirectory=D:\repos\my-electron-app\node_modules\@dennisameling\keytar-temp
• build native dependency from sources name=@dennisameling/keytar-temp
version=7.4.100-beta
platform=win32
arch=x64
napi= �
reason=prebuild-install failed with error (run with env DEBUG=electron-builder to get more information)
error=prebuild-install info begin Prebuild-install version 6.0.1
prebuild-install WARN This package does not support N-API version 11.2.3
prebuild-install WARN install prebuilt binaries enforced with --force!
prebuild-install WARN install prebuilt binaries may be out of date!
prebuild-install info looking for cached prebuild @ C:\Users\Dennis\AppData\Roaming\npm-cache\_prebuilds\a79a75-keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
prebuild-install http request GET https://github.com/dennisameling/node-keytar/releases/download/v7.4.100-beta/keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
prebuild-install http 404 https://github.com/dennisameling/node-keytar/releases/download/v7.4.100-beta/keytar-temp-v7.4.100-beta-napi-v11.2.3-win32-x64.tar.gz
prebuild-install WARN install No prebuilt binaries found (target=11.2.3 runtime=napi arch=x64 libc= platform=win32)
• execute command command='C:\Program Files\nodejs\node.exe' 'C:\Users\Dennis\AppData\Roaming\nvm\v14.15.5\node_modules\npm\bin\npm-cli.js' rebuild --verbose @dennisameling/keytar-temp@7.4.100-beta
workingDirectory=
⨯ cannot execute cause=exit status 1
So you're right, we do need to pass the target N-API version. Just for info, prebuild-install also looks at binary.napi_versions in the package's config, so the approach you took here should be totally valid.
Looking only at the first napi_version in the array
This approach is probably OK for now - in most cases, like you mentioned, it should take the lowest available N-API version from binary.napi_versions which should be fine. Looking forward, I already tried to do some research if we can do something similar to https://github.com/inspiredware/napi-build-utils/blob/master/index.js#L166-L179, but then for Electron.
The problem is this:
- When running an Electron application, Electron's available N-API version can be retrieved with
process.versions.napi, just like in Node itself. It returns the highest supported N-API version;6for example. - However,
prebuild-installleverages Electron'spackage.jsonto get the Electron version, and uses that to set itstargetversion. The highest supported N-API version isn't mentioned in thepackage.json, so AFAIK there's no way to retrieve Electron's N-API version without running Electron itself first.lovell/sharpworked around this by explicitly settingnpm_config_target=3in theirpackage.json. However, this won't help them as soon as they need to support multiple N-API versions.
I probably could create an issue in the electron repo for this - we could ask for a static way to receive the highest supported N-API for the installed Electron version. What do you think?
There was a problem hiding this comment.
Yeah an issue with electron would be good. Doesn't really help us here right now as that will only make it into the next version, but at some point in the future it will. They already have electron --version and electron --abi, so adding an electron --napi would be reasonable.
Until that is a thing, we could spin up electron with a very minimal script to extract the napi version
eg electron napi-version.js
napi-version.js:
console.log(process.versions.napi)
process.exit(0)
This wouldn't be hard to do, but I think it would be more appropriate to do on the js side, and feed it in alongside the platform and architecture.
There was a problem hiding this comment.
Issue created in electron/electron: electron/electron#27842
|
@develar any chance of getting this moving? |
|
Thanks for your help and fixing this bug! |
|
3.5.13 is published. |
|
Sorry to add to the noise here. I just have a stupid question. I maintain an Electron app that uses Sharp, and just ran into the issue described in #47 (comment). How can I track when this fixed PR will have made its way to production use in electron-builder? |
|
I missed that this had been published. Looks like a pr is needed to one of the electron-builder packages to use this new version, then it should be included in the next release there. |
fixes #46
Before:
After:
I don't know why the
napi=bit in the log line is blank, I believe I am using the correct zap method to convert it, but it ends up blank even when there are values. Do you have any ideas on what I did wrong?