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

Clean up include path and flag-check #152

Closed
wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

Fixes: #139

@gabrielschulhof
Copy link
Contributor Author

Note: I couldn't test this with versions of node other than 8.6.0, because node-addon-api doesn't currently build on those versions (#142).

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

I tried this with 8.6 (binary distro from nodejs.org) and get this error:

bash-4.2$ npm test

> node-addon-api@1.0.0 pretest /home/mhdawson/newpull/land/node-addon-api
> node-gyp rebuild -C test

make: Entering directory `/home/mhdawson/newpull/land/node-addon-api/test/build'
  SOLINK_MODULE(target) Release/obj.target/../src/nothing.node
  COPY Release/nothing.node
  CXX(target) Release/obj.target/binding/arraybuffer.o
../arraybuffer.cc:1:18: fatal error: napi.h: No such file or directory
 #include "napi.h"
                  ^
compilation terminated.
make: *** [Release/obj.target/binding/arraybuffer.o] Error 1
make: Leaving directory `/home/mhdawson/newpull/land/node-addon-api/test/build'
gyp ERR! build error
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/home/mhdawson/newpull/land/node-bin/node/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at emitTwo (events.js:125:13)
gyp ERR! stack     at ChildProcess.emit (events.js:213:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
gyp ERR! System Linux 3.10.0-327.18.2.el7.x86_64
gyp ERR! command "/home/mhdawson/newpull/land/node-bin/node/bin/node" "/home/mhdawson/newpull/land/node-bin/node/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild" "-C" "test"
gyp ERR! cwd /home/mhdawson/newpull/land/node-addon-api/test
gyp ERR! node -v v8.6.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok
npm ERR! Test failed.  See above for more details.

Without the change in this PR, the test build ok.

@mhdawson
Copy link
Member

@gabrielschulhof am I doing something wrong ?

@mhdawson
Copy link
Member

@gabrielschulhof ping.

node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

Fixes: nodejs#139
@gabrielschulhof
Copy link
Contributor Author

@mhdawson all fixed now.

@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

@gabrielschulhof thanks, landing now.

mhdawson pushed a commit that referenced this pull request Nov 2, 2017
node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

PR-URL: #152
Fixes: #139
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

Landed as 5a5920a

@mhdawson mhdawson closed this Nov 2, 2017
@gabrielschulhof gabrielschulhof deleted the fix-include branch December 27, 2018 18:01
@gabrielschulhof gabrielschulhof restored the fix-include branch December 28, 2018 20:27
@gabrielschulhof gabrielschulhof deleted the fix-include branch February 13, 2019 23:28
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

PR-URL: nodejs/node-addon-api#152
Fixes: nodejs/node-addon-api#139
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

PR-URL: nodejs/node-addon-api#152
Fixes: nodejs/node-addon-api#139
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

PR-URL: nodejs/node-addon-api#152
Fixes: nodejs/node-addon-api#139
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
node_api.h must be included from the normal node include path when
we can use the built-in API.

The variable needsFlag must be true from version 8.0.0 inclusive until
version 8.6.0 non-inclusive.

PR-URL: nodejs/node-addon-api#152
Fixes: nodejs/node-addon-api#139
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 this pull request may close these issues.

2 participants