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.

Sign up for GitHub

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

Jump to bottom

[v10.x] Backport N-API 6 to v10.x #32488

Closed
wants to merge 2 commits into from
Closed

[v10.x] Backport N-API 6 to v10.x #32488

wants to merge 2 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

Checklist
  • 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

Gabriel Schulhof and others added 2 commits March 25, 2020 10:45
n-api: correct instance data tests
07c0385 
When instance data was backported, some of the tests ended up in a
location where they do not get run. This moves the tests into
test/addons-napi, merging them with existing tests therein, thereby
ensuring that they do get run.
@legendecas
n-api: define release 6
e1e2c2d 
Mark all N-APIs that have been added since version 5 as stable.

PR-URL: nodejs#32058
Fixes: nodejs/abi-stable-node#393
Co-Authored-By: legendecas <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof added c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. node-api Issues and PRs related to the Node-API. labels Mar 25, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@mhdawson
Copy link
Member

@gabrielschulhof did the commits apply cleanly?

@gabrielschulhof
Copy link
Contributor Author

@mhdawson I have to move the tests out of test/node-api/test_instance_data into test/addons-napi/test_instance data and merge the code, because the former are not part of the v10.x source tree. We added test/node-api after splitting up js-native-api and node-api. In v10.x they are still together.

Thus, I have an original commit that corrects this in addition to the mark-v6-as-stable commit that is backported from master. The commit should have been squashed into the commit backporting the instance data but I had forgotten to make the change at the time.

@mhdawson
Copy link
Member

@gabrielschulhof thanks.

Not a big deal but I noted that the order of the functions in the NAPI-VERSION >= block is different in 10.x versus master. (napi_get_all_property_names is at the end in master but not in 10.x)

@mhdawson
Copy link
Member

Sorry got that backwards, it is at the end in 10.x but not in master.

mhdawson
mhdawson approved these changes Mar 26, 2020
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

BethGriggs pushed a commit that referenced this pull request Apr 6, 2020
@BethGriggs
n-api: correct instance data tests
239377b 
When instance data was backported, some of the tests ended up in a
location where they do not get run. This moves the tests into
test/addons-napi, merging them with existing tests therein, thereby
ensuring that they do get run.

PR-URL: #32488
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 6, 2020
@legendecas @BethGriggs
n-api: define release 6
e9c590e 
Mark all N-APIs that have been added since version 5 as stable.

PR-URL: #32058
Backport-PR-URL: #32488
Fixes: nodejs/abi-stable-node#393
Co-Authored-By: legendecas <[email protected]>
Signed-off-by: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@BethGriggs
Copy link
Member

Landed in 239377b, e9c590e

@BethGriggs BethGriggs closed this Apr 6, 2020
@BethGriggs BethGriggs mentioned this pull request Apr 7, 2020
Merged
@gabrielschulhof
Copy link
Contributor Author

@BethGriggs looks like these modifications somehow did not end up in the release tarball, although they are tagged correctly as v10.20.0. Please see #32755!

@gabrielschulhof gabrielschulhof deleted the backport-n-api-6-to-v10.x branch April 21, 2020 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gabrielschulhof @nodejs-github-bot @mhdawson @BethGriggs @legendecas