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

Expose napi version as a node-gyp variable #1745

Closed
josephg opened this issue May 7, 2019 · 7 comments
Closed

Expose napi version as a node-gyp variable #1745

josephg opened this issue May 7, 2019 · 7 comments

Comments

@josephg
Copy link

josephg commented May 7, 2019

For my module I want to do conditional compilation based on node-gyp feature support.

Unfortunately there's currently no nice way to figure out at node-gyp configuration time which napi version is supported. I'm currently doing this:

  'variables': {
    'napi_version': '<!(node -e "console.log(process.versions.napi)")',
  },

But this checks napi support in the host version of nodejs, not in the nodejs build target version.

@mhdawson
Copy link
Member

We discussed in the N-API team meeting today.

Jim mentioned that the N-API team (and by that I mean Jim :)) added the concept a Napi-version to node-pre-gyp and prebuild. @jschlight will take a look at whether something similar makes sense for node-gyp.

@richardlau
Copy link
Member

For my module I want to do conditional compilation based on node-gyp feature support.

Unfortunately there's currently no nice way to figure out at node-gyp configuration time which napi version is supported. I'm currently doing this:

  'variables': {
    'napi_version': '<!(node -e "console.log(process.versions.napi)")',
  },

But this checks napi support in the host version of nodejs, not in the nodejs build target version.

Is the desired variable NAPI_VERSION from node_version.h? If so something like this:

  'variables': {
    'napi_version': '<!(awk \'$1 == "#define" && $2 == "NAPI_VERSION" {print $3}\' <(node_root_dir)/include/node/node_version.h)'
  },

(doesn't have to be awk -- the main thing is <(node_root_dir)/include/node/node_version.h) which is the header files being compiled against (node_root_dir is set by node-gyp)).

@josephg
Copy link
Author

josephg commented May 13, 2019

Yeah NAPI_VERSION is what I’m after. That’d do the trick, although it wouldn’t work on windows because there’s no awk there. For my project I’ve landed on an awful workaround of just making a bunch of indirect source files that conditionally include the right source files. It’d be nice to sort this out via gyp though.

@richardlau
Copy link
Member

Maybe something like the following for cross-platform (looks messy because of the needed escaping):

  'variables': {
    'napi_version': '<!(node -p "require(\'fs\').readFileSync(\'<(node_root_dir)/include/node/node_version.h\', \'utf8\').match(/NAPI_VERSION\\s*(\\d+)/)[1]")'
  },

Definitely not opposed to having node-gyp define a napi_version variable for use in binding.gyp files, but bear in mind it would take a while to filter through to dependents, e.g. npm (and, for example, the default version of npm in, say, Node.js 8, and by extension its version of node-gyp is unlikely to get updated).

@jschlight
Copy link

node-pre-gyp and prebuild permit the user to request multiple builds using node-gyp. For both tools we defined a new node-gyp variable napi_build_version, accessible in the binding.gyp file, that communicates the N-API version the tool is requesting to build.

I think the work we did there does not apply to this situation.

@bnoordhuis
Copy link
Member

node-gyp isn't the right place to embed that logic. If node exposes a napi_build_version through its common.gypi, node-gyp will automagically make it available to add-ons. I suggest moving this to nodejs/node.

@bnoordhuis
Copy link
Member

There's an open pull request over at nodejs/node that is this close to landing so I'll go ahead and close out this issue.

pull bot pushed a commit to SimenB/node that referenced this issue Jul 2, 2019
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 pushed a commit to nodejs/node that referenced this issue Jul 2, 2019
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 pushed a commit to nodejs/node that referenced this issue Jul 2, 2019
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>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Sep 19, 2020
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>
richardlau pushed a commit to nodejs/node that referenced this issue Oct 7, 2020
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
Backport-PR-URL: #35266
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants