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

Build for napi runtime when node-addon-api and napi_versions found #18

Merged
merged 3 commits into from
Jun 28, 2019

Conversation

lovell
Copy link
Member

@lovell lovell commented Jun 25, 2019

Once a native module has made the leap to using N-API via node-addon-api, attempting to build for node and electron ABI runtimes will fail.

This is, in part, due to a lack of a one-to-one mapping between node/electron ABIs and napi ABIs e.g. node ABI 57 maps to napi ABI 1, 2, 3 and 4.

This change is fully backwards compatible. Existing modules that do not both depend on node-addon-api and specify napi_versions carry on as before.

@lovell lovell added the enhancement New feature or request label Jun 25, 2019
build-targets.js Outdated Show resolved Hide resolved
@lovell
Copy link
Member Author

lovell commented Jun 26, 2019

I've updated this PR so the napi runtime is additive. The internals have been corrected to use target rather than abi to ensure we're passing the expected value to prebuild's -t flag, which then allows node-addon-api to build correctly.

I tested this with release candidate v3.0.0-rc1 of the farmhash module and it generated the expected prebuilds when using Node 10 and with napi_versions = [2, 3, 4] (napi 4 was correctly ignored as this requires Node 12):

farmhash-v3.0.0-rc1-electron-v64-linux-x64.tar.gz
farmhash-v3.0.0-rc1-electron-v69-linux-x64.tar.gz
farmhash-v3.0.0-rc1-electron-v70-linux-x64.tar.gz
farmhash-v3.0.0-rc1-napi-v2-linux-x64.tar.gz
farmhash-v3.0.0-rc1-napi-v3-linux-x64.tar.gz
farmhash-v3.0.0-rc1-node-v64-linux-x64.tar.gz

@vweevers
Copy link
Member

@lovell Why is it a requirement to have node-addon-api in your dependencies? What happens when you don't use that C++ API?

@lovell
Copy link
Member Author

lovell commented Jun 28, 2019

It's optional but highly recommended; happy to remove that check if napi_versions is considered a strong enough indicator.

I think of node-addon-api as the N-API equivalent of nan, without which maintaining native modules would be a(n even greater) burden.

@vweevers
Copy link
Member

There are quite a few modules that don't use node-addon-api, but napi-macros (many of these also don't use prebuild[-ci], but that's besides my point that node-addon-api is not the only solution).

This change corrects the internals to work with target versions rather
than ABI versions as this is what prebuild expects with its -t flag.
@lovell
Copy link
Member Author

lovell commented Jun 28, 2019

napi-macros looks useful, I hadn't seen that before, thanks for the tip. I've updated this PR to remove the dependency check and rely on the presence of napi_versions only for now.

@vweevers
Copy link
Member

Before this PR, we didn't support N-API at all, correct? Meaning we can safely release this as semver-minor.

@lovell
Copy link
Member Author

lovell commented Jun 28, 2019

That's correct, this change can be part of a minor increment.

Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two nitpicks, LGTM otherwise.

build-targets.js Outdated Show resolved Hide resolved
build-targets.js Outdated Show resolved Hide resolved
@vweevers vweevers added the semver-minor New features that are backward compatible label Jun 28, 2019
lovell and others added 2 commits June 28, 2019 12:57
Co-Authored-By: Vincent Weevers <mail@vincentweevers.nl>
Co-Authored-By: Vincent Weevers <mail@vincentweevers.nl>
@vweevers vweevers merged commit 603a1e1 into prebuild:master Jun 28, 2019
@vweevers
Copy link
Member

3.1.0

@lovell lovell deleted the detect-napi-runtime branch June 28, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor New features that are backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants