add CI and fix current build issue by gengjiawen · Pull Request #104 · nodejs/node-addon-examples

gengjiawen · 2019-08-06T13:07:12Z

Update:
https://travis-ci.org/gengjiawen/node-addon-examples

Now all test pass Node.js 8, 10.

gengjiawen · 2019-08-06T14:25:17Z

@targos Can you enable travis for this repo too ? Thanks.

gengjiawen · 2019-08-07T09:37:14Z

on node-addon-examples/async_work_thread_safe_function/node-api looks like the api is introduced by on v10.6.0

https://nodejs.org/dist/latest-v12.x/docs/api/n-api.html#n_api_napi_threadsafe_function

gabrielschulhof

We should definitely have a CI on this repo, but ideally the trigger would be changes to Node.js, not changes to this repo.

gengjiawen · 2019-08-10T06:59:45Z

We should definitely have a CI on this repo, but ideally the trigger would be changes to Node.js, not changes to this repo.

Do you mean test against Node.js nightly ?

gabrielschulhof · 2019-08-10T08:14:07Z

@gengjiawen against nightly would be great, but also against the latest LTS versions when they are released. I'm not sure if we can trigger Travis like that though.

gengjiawen · 2019-08-10T08:17:50Z

@gengjiawen against nightly would be great, but also against the latest LTS versions when they are released. I'm not sure if we can trigger Travis like that though.

Maybe related to nodejs/Release#422.

But I think current solution will works and prevent regression.

NickNaso

SGTM I left two comments.

mhdawson · 2019-08-14T18:19:15Z

  "dependencies": {
    "bindings": "~1.2.1"
  },
+  "engines": {


Same comment as before, the real dependency is N-API version 4 which is supported by some version of 8.X as well. In any case this is still a good step.

@mhdawson you're right and my recent experience push me to say that the most useful value to check if a feature is supported or not is the N-API version.

The problem is that it's not possible to check if a Node.js version is greater than XXX because for example the 8.16.0 has napi_thread_safe_function instead the 10.0.0 not.

I was wondering if we could specify multiple ranges maybe

">= 8.16.0 < 9.0.0 || >=10.6.0"

but not sure if that is allowed by the syntax.

Yes it's possible to specify multiple ranges. What is specified on the engines field has effect only if the engine-strict config flag is set.

npm --engine-strict install

The example failed on macOs Node.js 8.16.0, can you run it in other OS ?

gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ] CC(target) Release/obj.target/binding/binding.o ../binding.c:185:16: warning: type specifier missing, defaults to 'int' [-Wimplicit-int] /*napi_value*/ NAPI_MODULE_INIT(/*napi_env env, napi_value exports*/) { ^ ../binding.c:204:33: error: use of undeclared identifier 'env' assert(napi_define_properties(env, exports, 1, &start_work) == napi_ok); ^ ../binding.c:204:38: error: use of undeclared identifier 'exports' assert(napi_define_properties(env, exports, 1, &start_work) == napi_ok); ^ ../binding.c:208:20: error: use of undeclared identifier 'env' assert(napi_wrap(env, ^ ../binding.c:209:20: error: use of undeclared identifier 'exports' exports, ^ ../binding.c:216:10: error: use of undeclared identifier 'exports' return exports; ^ 1 warning and 5 errors generated.

Looks like no macro NAPI_MODULE_INIT on 8.x https://github.com/nodejs/node/blob/v8.x/src/node_api.h

Refactor Module Initialization to NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) works on 8.x.

Should we do it ? cc @gabrielschulhof

NAPI_MODULE_INIT being missing on v8.x may be an oversight as we expect same code to work across all the LTS versions subject to the NAPI_VERSION supported. Having said that,. not sure we'll be able to get that into the last v8.x release. Also need @gabrielschulhof to comment on whether than even makes sense. For now if we can refactor, maybe with an #ifdef so the example still shows the way we would want people to do it post 8.x ?

NAPI_MODULE now works on all platform, #ifdef may not be necessary.

node-addon-examples/1_hello_world/napi/hello.cc

Line 23 in 31f8ef9

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

mhdawson

LGTM with a comment

mhdawson

LGTM

gengjiawen · 2019-08-16T07:46:03Z

LGTM

Do you have permission to enable travis for this repo ? We need to enable it before this got merged.

mhdawson · 2019-08-16T18:30:59Z

@gengjiawen I've never done that before. What needs to be done?

gengjiawen · 2019-08-17T01:01:20Z

@gengjiawen I've never done that before. What needs to be done?

Need someone with admin to nodejs to enable it in travis-ci.

mhdawson · 2019-08-20T20:50:59Z

I have the admin rights I just don't know where in the github ui to enable. I'm sure I can figure it out but if you already know and can tell me that will save me some time.

gengjiawen · 2019-08-21T00:53:42Z

I have the admin rights I just don't know where in the github ui to enable. I'm sure I can figure it out but if you already know and can tell me that will save me some time.

https://travis-ci.org/account/repositories, on left side menu, there should be ORGANIZATIONS, in there click Node.js, you should find the repo and enable it.

mhdawson · 2019-08-22T15:47:45Z

@gengjiawen sorry that I don't have enough context/background but I'm confused. We already use travis for some builds (for example: https://github.com/nodejs/node/pull/29263/checks?check_run_id=200242495) so I was not expecting we needed to enable anything on the travis side, instead some configuration on the node-addon-examples repo instead.

mhdawson · 2019-08-22T16:24:43Z

Found the config within the Node.js org, have enabled it for this repo now.

gengjiawen · 2019-08-23T01:58:50Z

Found the config within the Node.js org, have enabled it for this repo now.

Can we merge this and open an issue for #104 (comment) ?

mhdawson · 2019-08-23T14:23:05Z

@gengjiawen

Can we merge this and open an issue for #104 (comment) ?

Sounds good to me.

gengjiawen · 2019-08-24T02:26:36Z

@gengjiawen

Can we merge this and open an issue for #104 (comment) ?

Sounds good to me.

#108

add CI

34f8df1

gengjiawen force-pushed the ci branch from aea8a8e to 34f8df1 Compare August 6, 2019 13:12

add CI

bb05c28

gengjiawen force-pushed the ci branch from ba1f7b1 to bb05c28 Compare August 6, 2019 13:25

add CI

9ab21e8

gengjiawen marked this pull request as ready for review August 6, 2019 13:33

gengjiawen requested a review from mhdawson August 6, 2019 13:34

gengjiawen mentioned this pull request Aug 6, 2019

Should CI test include various linux distros. #105

Closed

add CI

6d68d1a

fix 7

d962031

gengjiawen changed the title ~~add CI~~ add CI and fix current build issue Aug 6, 2019

fix 8

2802418

gengjiawen requested a review from gabrielschulhof August 6, 2019 14:54

gengjiawen added 2 commits August 10, 2019 08:43

fix 9

80dd772

fix 10

c2d7625

gabrielschulhof reviewed Aug 10, 2019

View reviewed changes

gengjiawen requested a review from NickNaso August 10, 2019 10:26

NickNaso reviewed Aug 12, 2019

View reviewed changes

Comment thread .gitignore

Comment thread 6_object_wrap/nan/package.json

Update .gitignore

f642612

mhdawson reviewed Aug 14, 2019

View reviewed changes

Comment thread async_work_thread_safe_function/node-api/package.json

mhdawson reviewed Aug 14, 2019

View reviewed changes

mhdawson approved these changes Aug 14, 2019

View reviewed changes

mhdawson mentioned this pull request Aug 22, 2019

Enable Travis for node-addon-examples nodejs/build#1899

Closed

gengjiawen mentioned this pull request Aug 24, 2019

Fix api compatibility #108

Closed

gengjiawen merged commit 0f3ea58 into nodejs:master Aug 24, 2019

gengjiawen deleted the ci branch August 24, 2019 02:27

Uh oh!

Conversation

gengjiawen commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengjiawen commented Aug 6, 2019

Uh oh!

gengjiawen commented Aug 7, 2019

Uh oh!

gabrielschulhof left a comment

Choose a reason for hiding this comment

Uh oh!

gengjiawen commented Aug 10, 2019

Uh oh!

gabrielschulhof commented Aug 10, 2019

Uh oh!

gengjiawen commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickNaso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengjiawen Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

gengjiawen commented Aug 16, 2019

Uh oh!

mhdawson commented Aug 16, 2019

Uh oh!

gengjiawen commented Aug 17, 2019

Uh oh!

mhdawson commented Aug 20, 2019

Uh oh!

gengjiawen commented Aug 21, 2019

Uh oh!

mhdawson commented Aug 22, 2019

Uh oh!

mhdawson commented Aug 22, 2019

Uh oh!

gengjiawen commented Aug 23, 2019

Uh oh!

mhdawson commented Aug 23, 2019

Uh oh!

gengjiawen commented Aug 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gengjiawen commented Aug 6, 2019 •

edited

Loading

gengjiawen commented Aug 10, 2019 •

edited

Loading

gengjiawen Aug 16, 2019 •

edited

Loading