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: expose napi_build_version variable to native addons #27835

Closed
wants to merge 6 commits into from

Conversation

@NickNaso
Copy link
Member

commented May 23, 2019

Expose napi_build_version to allow node-gyp to make it
available for addons.
Fixes: nodejs/node-gyp#1745
Refs: nodejs/abi-stable-node#371

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@NickNaso NickNaso referenced this pull request May 23, 2019
@lpinca
lpinca approved these changes May 23, 2019
Copy link
Member

left a comment

SGTM

@lpinca

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@NickNaso can you please remove "build:" and capitalize "ensure" in commit message body?

Expose `napi_build_version` to allow `node-gyp` to make it available for
addons.
build: expose napi_build_version variable to native addons
Expose `napi_build_version` to allow `node-gyp` to make it
available for addons.
Fixes: nodejs/node-gyp#1745
Refs: nodejs/abi-stable-node#371
@mhdawson

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@NickNaso is there any way to have this be generated from where the value is defined? Just worried about keeping the 2 in sync. I wonder if something the in the makefiles/build steps could update it? MIght be a bad idea if we don't modify the build files already but thought I'd ask to see if anybody else has any suggestions/comments as well.

@NickNaso

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@mhdawson I will investigate later or tomorrow what you suggested because it's a more robust solution. If anyone want help all feedback are welcome.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 23, 2019

configure.py could write it to config.gypi, that's also picked up by node-gyp. Another benefit: it'll show up in process.config as process.config.variables.napi_build_version; useful for post-install scripts.

There's precedence, see tools/getmoduleversion.py and its corresponding import and usage in configure.py. That's what generates process.config.variables.node_module_version.

@mhdawson

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@bnoordhuis thanks for the great pointers.

@addaleax

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

A test with assert.strictEqual(process.config.variables.napi_build_version, process.versions.napi); should work if @bnoordhuis’ suggestion is implemented, as far as keeping the versions up to date is concerned, right?

@NickNaso

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2019

Thanks all for suggestions I will update the PR very soon.

@legendecas
Copy link
Contributor

left a comment

There is a definition of NAPI_VERSION in node_version.h. Should it be derived from the one in js_native_api.h too?

@NickNaso

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@legendecas
I didn't know that node_verssion.h contains the definition of NAPI_VERSION and I think that it's more appropriate use this file to retrieve the version of N-API. I will provide to update the PR.

@nodejs-github-bot

This comment has been minimized.

@legendecas
Copy link
Contributor

left a comment

Wondering the difference of NAPI_VERSION between node_version.h and js_native_api.h. Should they both be updated on version bumping?

@NickNaso

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@legendecas I think that we need to update both version in node_version.h and in js_native_api.h, but your concern is about the fact the we need to change the version in two places. Now I'm thinking that if we use js_native_api.h to retrive the N-API version could be more appropriate because I added a test so if the test fail this means there is a bad N-API version set in js_native_api.h or node_version.h. What do you think?

@NickNaso

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Hi everyone,
like I explained in the previous comment now we have two places (node_version.h and js_native_api.h) where we need to update the N-API version and this could not be ideal. In these days I tried to find a solution that now I want propose:

  • Create a new file called js_native_api_version.h and define NAPI_BASE_VERSION
  • Import js_native_api_version.h in node_version.h and js_native_api.h and then use the NAPI_BASE_VERSION

I executed the tests and it seems to work, but I don't know if it could be the right solution.
Someone have an opinion or feedback about that?

@nodejs/n-api

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

The two definitions of N-API version are actually a bit different.

The one in node_version.h is the version which the Node binary being built supports.

The one in js_native_api_version.h controls which version will be used by default when compiling a native addon. For example, if all LTS versions support N-API 4, but the latest support N-API 5 we still may want to have the default be N-API 4 be the default so that the addon can compile across all of the LTS versions. If the addon developer specifically wants to use functions in N-API 5 they can do that by setting NAPI_VERSION to 5 knowing that they have specifically depended on that version.

Might have been better if we had named them differently as I can't remember if there was any reason they needed to be the same.

My thought is that having them be separate is useful but that we should have more comments to explain what I've just said in the code.

@jschlight

This comment has been minimized.

Copy link

commented Jun 10, 2019

If I understand correctly, the NAPI_VERSION value is meant to be optionally overridden using build tools.

For example, node-pre-gyp (N-API Doc) and prebuild (N-API Doc) are designed to fire off multiple builds, one for each N-API version supported by the native module. The NAPI_VERSION value is used to communicate to the C/C++code which N-API version the code is being built against.

I'm not clear what effect, if any, NAPI_VERSION should have on napi_build_version if the value of NAPI_VERSION set externally by a build tool.

Edit: Reviewing the documentation linked above, it looks like for node-pre-gyp and prebuild the NAPI_VERSION is derived directly from napi_build_version. So with these two tools we're good.

src/js_native_api.h Outdated Show resolved Hide resolved
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@mhdawson NAPI_VERSION as defined in js_native_api.h is defined softly. Thus, if you wish to build against whichever version your copy of Node.js supports, you can

#include "node_version.h"
#include "js_native_api.h"
@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@mhdawson that's probably why they have the same name.

@mhdawson
Copy link
Member

left a comment

LGTM

@nodejs-github-bot

This comment has been minimized.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@bnoordhuis
Copy link
Member

left a comment

LGTM % style nit. Thanks for doing this!

edit: the CI failures are all infrastructural, by the way.

configure.py Outdated Show resolved Hide resolved
@NickNaso

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Hi everybody is there a chance to merge this PR?

@nodejs-github-bot

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Landed in 9868126.

pull bot pushed a commit to SimenB/node that referenced this pull request Jul 2, 2019
build: expose napi_build_version variable
Expose `napi_build_version` to allow `node-gyp` to make it
available for building native addons.

Fixes: nodejs/node-gyp#1745
Refs: nodejs/abi-stable-node#371
PR-URL: nodejs#27835
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos added a commit that referenced this pull request Jul 2, 2019
build: expose napi_build_version variable
Expose `napi_build_version` to allow `node-gyp` to make it
available for building native addons.

Fixes: nodejs/node-gyp#1745
Refs: nodejs/abi-stable-node#371
PR-URL: #27835
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos added a commit that referenced this pull request Jul 2, 2019
build: expose napi_build_version variable
Expose `napi_build_version` to allow `node-gyp` to make it
available for building native addons.

Fixes: nodejs/node-gyp#1745
Refs: nodejs/abi-stable-node#371
PR-URL: #27835
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@targos targos referenced this pull request Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.