★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/32488Nouvelle recherche | Portail wanayoo
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

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

@gabrielschulhof gabrielschulhof commented Mar 25, 2020

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 Mar 25, 2020
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 <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 25, 2020

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

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Mar 25, 2020

@gabrielschulhof did the commits apply cleanly?

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof gabrielschulhof commented Mar 25, 2020

@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

@mhdawson mhdawson commented Mar 26, 2020

@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

@mhdawson mhdawson commented Mar 26, 2020

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 changes
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 30, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 2, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 2, 2020

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

BethGriggs pushed a commit that referenced this issue 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 <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
BethGriggs pushed a commit that referenced this issue 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 <legendecas@gmail.com>
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@BethGriggs
Copy link
Member

@BethGriggs BethGriggs commented Apr 6, 2020

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

@gabrielschulhof gabrielschulhof commented Apr 10, 2020

@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 Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs

@legendecas legendecas

@mhdawson mhdawson

Assignees
No one assigned
Labels
c++ node-api semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@gabrielschulhof @nodejs-github-bot @mhdawson @BethGriggs @legendecas