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

[v14.x] node-api: handle no support for external buffers #45616

Closed

Conversation

mhdawson
Copy link
Member

Refs: electron/electron#35801 Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:

  • hide the methods to create external buffers so they can avoid using them if they want the broadest compatibility.
  • call the methods that create external buffers at runtime to check if external buffers are supported and either use them or not based on the return code.

Signed-off-by: Michael Dawson mdawson@devrus.com

PR-URL: #45181
Reviewed-By: Chengzhong Wu legendecas@gmail.com
Reviewed-By: Minwoo Jung nodecorelab@gmail.com

Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: nodejs#45181
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v14.x labels Nov 24, 2022
@mhdawson
Copy link
Member Author

mhdawson commented Nov 24, 2022

@legendecas if you could review this backport that would be great.

@richardlau richardlau changed the title node-api: handle no support for external buffers [v14.x] node-api: handle no support for external buffers Nov 24, 2022
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -928,6 +928,10 @@ napi_status napi_create_external_buffer(napi_env env,
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

#if defined(V8_ENABLE_SANDBOX)
Copy link
Member

Choose a reason for hiding this comment

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

I don't find this definition on v14.x. Maybe we can simply remove this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

That could make sense, I think we wanted the backport so that people could compile on the oldest LTS and use the value for the error code.

They question is whether it's better to keep it closer to the original PR even if the define will never be set or to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau do you have an opinion from the Releaser perspective? I'm happy either way. We can remove the define or leave it in to make it a "backport" versus a "backport" with modifications whichever is easier/preferrable for getting it landed.

Copy link
Member

Choose a reason for hiding this comment

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

@mhdawson No preference either way. I think it's very unlikely V8_ENABLE_SANDBOX would be introduced to Node.js 14 if it isn't already there given that Node.js 14 is in maintenance and will go End-of-Life in April.

cc @nodejs/lts

Copy link
Member Author

Choose a reason for hiding this comment

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

@legendecas since @richardlau figures there is no preference from the Releasers perspective I don't don't feel strongly either. I'm happy to leave as it as I don't think it hurts anything but also happy to remove that i you think that's best.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with both ways. Thanks for the clarification.

@@ -928,6 +928,10 @@ napi_status napi_create_external_buffer(napi_env env,
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

#if defined(V8_ENABLE_SANDBOX)
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with both ways. Thanks for the clarification.

@mhdawson
Copy link
Member Author

@richardlau I'll leave as is given the discussion.

richardlau pushed a commit that referenced this pull request Dec 7, 2022
Refs: electron/electron#35801
Refs: nodejs/abi-stable-node#441

Electron recently dropped support for external
buffers. Provide a way for addon authors to:
- hide the methods to create external buffers so they can
  avoid using them if they want the broadest compatibility.
- call the methods that create external buffers at runtime
  to check if external buffers are supported and either
  use them or not based on the return code.

Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #45181
Backport-PR-URL: #45616
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@richardlau
Copy link
Member

Landed in 2dbeb88.

@richardlau richardlau closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants