build: expose napi_build_version variable to native addons

[NickNaso]

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

[lpinca]

SGTM

[lpinca]

@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. 

[mhdawson]

@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]

@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]

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]

@bnoordhuis thanks for the great pointers.

[addaleax]

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]

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

[legendecas]

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]

@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]

CI: https://ci.nodejs.org/job/node-test-pull-request/23606/

[legendecas]

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

[NickNaso]

@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]

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]

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]

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.

[gabrielschulhof]

@mhdawsonNAPI_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]

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

[mhdawson]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/23944/

[mhdawson]

CI run: https://ci.nodejs.org/job/node-test-pull-request/23944/

[mhdawson]

Rebuild - https://ci.nodejs.org/job/node-test-commit/29527/

[bnoordhuis]

LGTM % style nit. Thanks for doing this!

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

[NickNaso]

Hi everybody is there a chance to merge this PR?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/24189/

[gabrielschulhof]

Landed in 9868126.