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

[v14.x] Backport 33570 and 33508 to v14.x #33817

Closed
wants to merge 2 commits into from
Closed

[v14.x] Backport 33570 and 33508 to v14.x #33817

wants to merge 2 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

#33508 cannot be backported directly, but must be preceded with a backport of #33570.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Gabriel Schulhof and others added 2 commits June 9, 2020 16:23
n-api: remove napi_env::CallIntoModuleThrow
a405569 
Give `napi_env::CallIntoModule` the thrower used by
`CallIntoModuleThrow` as its default second argument. That way we do
not need two different methods on `napi_env` for calling into the
addon.

PR-URL: nodejs#33570
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson
n-api: ensure scope present for finalization
b055a52 
Refs: nodejs/node-addon-api#722

Ensure a scope is on stack during finalization
as finalization functions can create JS Objects

Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#33508
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. v14.x labels Jun 10, 2020
@gabrielschulhof gabrielschulhof changed the title Backport 33570 and 33508 to v14.x [v14.x] Backport 33570 and 33508 to v14.x Jun 10, 2020
mhdawson
mhdawson approved these changes Jun 11, 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/31846/

@codebytere
Copy link
Member

@gabrielschulhof this fails to apply with git node land --backport 33817 on latest staging 🤔

Applying: n-api: remove `napi_env::CallIntoModuleThrow`
error: patch failed: src/js_native_api_v8.cc:267
error: src/js_native_api_v8.cc: patch does not apply
error: patch failed: src/js_native_api_v8.h:82
error: src/js_native_api_v8.h: patch does not apply
error: patch failed: src/node_api.cc:57
error: src/node_api.cc: patch does not apply
Patch failed at 0001 n-api: remove `napi_env::CallIntoModuleThrow`
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
--------------------------------------------------------------------------------
? The normal `git am` failed. Do you want to retry with 3-way merge? Yes
Applying: n-api: remove `napi_env::CallIntoModuleThrow`
Using index info to reconstruct a base tree...
M       src/js_native_api_v8.cc
M       src/js_native_api_v8.h
M       src/node_api.cc
Falling back to patching base and 3-way merge...
Auto-merging src/node_api.cc
Auto-merging src/js_native_api_v8.cc
CONFLICT (content): Merge conflict in src/js_native_api_v8.cc
Recorded preimage for 'src/js_native_api_v8.cc'
error: Failed to merge in the changes.
Patch failed at 0001 n-api: remove `napi_env::CallIntoModuleThrow`
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

@gabrielschulhof
Copy link
Contributor Author

@codebytere it looks like these two have already landed as 524daf8 and e83642f.

@gabrielschulhof gabrielschulhof deleted the backport-33508-to-v14.x branch July 10, 2020 18:44
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

@addaleax addaleax Awaiting requested review from addaleax

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.
Projects
None yet
Milestone
No milestone
Development

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