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

When was napi_detach_arraybuffer added? #33001

Closed
mafintosh opened this issue Apr 22, 2020 · 8 comments
Closed

When was napi_detach_arraybuffer added? #33001

mafintosh opened this issue Apr 22, 2020 · 8 comments
Labels
node-api Issues and PRs related to the Node-API. release-agenda Issues and PRs to discuss during the meetings of the Release team.

Comments

@mafintosh
Copy link
Member

  • Version: 12.16.2
  • Platform: mac
  • Subsystem: n-api

Trying to find which versions of Node that support the new experimental detach_arraybuffer api.

On the 12.x docs linked from the websites it says that the napi_detach_arraybuffer api was added in Node 12 and on the the 14.x docs it says it was added in Node 13.

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

Side note, I get an implicit declaration warning when trying to use the api as well in both latest 12 and 14 (code works tho), so unsure if that's a mistake I'm doing or not (all the other napi calls works fine and compile without errors).

Can try to investigate that part and open a separate issue.

../binding.c:27:3: warning: implicit declaration of function 'napi_detach_arraybuffer' is invalid in C99 [-Wimplicit-function-declaration]
  napi_detach_arraybuffer(env, argv[0]);
  ^
@himself65 himself65 added the node-api Issues and PRs related to the Node-API. label Apr 23, 2020
@mafintosh
Copy link
Member Author

I'm also getting a FATAL ERROR on 12 when I detach an externally created arraybuffer

FATAL ERROR: v8::ArrayBuffer::Detach Only detachable ArrayBuffers can be detached

@legendecas
Copy link
Member

legendecas commented Apr 24, 2020

The difference between v12 and v14 in doc is because of the backport strategy. We land the napi_detach_arraybuffer on master, while Node.js v12.x were already published with several versions. Later when we backport napi_detach_arraybuffer to v12 and publish a new version with napi_detach_arraybuffer, the doc in v12.x branch has to clarify which exact v12.x version napi_detach_arraybuffer was publish in.

On the contrary, first Node.js v14.x was published after we have landed napi_detach_arraybuffer on Node.js v13.

You see, it's two different releasing line that running in parallel so that we can get LTS support on v12.x.

The reason of the crash on v12 has been posted here #33022 (comment). I believe we'll get a fix soon :).

@addaleax
Copy link
Member

@nodejs/releasers and for this particular one @MylesBorins There’s something I don’t understand here, though… the REPLACEME → version replacement is part of the Release commit, in this case d20e494, and we cherry-pick that to master afaik (in this case 1c11ea4). Why don’t the versions get updated on master?

@targos
Copy link
Member

targos commented Apr 24, 2020

Because on master, the added: lines do not contain REPLACEME anymore. They are already set to some previous "Current" release number. When we cherry-pick from the 12.x release branch, we encounter a conflict and have to choose between added: v13.x.y and added: v12.x.y. For now, what we do is keep whatever is already on the master branch.

@addaleax addaleax added the release-agenda Issues and PRs to discuss during the meetings of the Release team. label Apr 24, 2020
@addaleax
Copy link
Member

For now, what we do is keep whatever is already on the master branch.

That’s not what should be happening here, imo… it’s definitely not what I had in mind when I introduced the REPLACEME tags.

I get that resolving the conflicts is some manual work, but it’s work that we should do, instead of having to manually fix up every single item à la #32639. If necessary, we could probably automate that conflict resolution to some degree.

@addaleax
Copy link
Member

@targos I guess that also means we should go through all LTS minors that have happened since the introduction of REPLACEME and fix the versions for those?

@targos
Copy link
Member

targos commented Apr 24, 2020

I honestly didn't know that the added field supported arrays.
Does it also work with entries in changes.version ?

I guess that also means we should go through all LTS minors that have happened since the introduction of REPLACEME and fix the versions for those?

Yeah, probably.

@addaleax
Copy link
Member

Does it also work with entries in changes.version ?

It should, yes:

const version = common.arrify(change.version).join(', ');

I guess that also means we should go through all LTS minors that have happened since the introduction of REPLACEME and fix the versions for those?

Yeah, probably.

Ok, I guess I can try to do that… there are only so many LTS Release commits, this should be pretty straightforward

addaleax added a commit to addaleax/node that referenced this issue Apr 27, 2020
When cherry-picking release commits for LTS releases into master,
the `REPLACEME` metadata can be taken over as well, to give users
a more accurate view of what is being released on which release line.

This addresses this problem for all previous LTS releases for which
this has not been done.

Fixes: nodejs#33001
BethGriggs pushed a commit that referenced this issue Apr 28, 2020
When cherry-picking release commits for LTS releases into master,
the `REPLACEME` metadata can be taken over as well, to give users
a more accurate view of what is being released on which release line.

This addresses this problem for all previous LTS releases for which
this has not been done.

Fixes: #33001

PR-URL: #33041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 28, 2020
When cherry-picking release commits for LTS releases into master,
the `REPLACEME` metadata can be taken over as well, to give users
a more accurate view of what is being released on which release line.

This addresses this problem for all previous LTS releases for which
this has not been done.

Fixes: #33001

PR-URL: #33041
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API. release-agenda Issues and PRs to discuss during the meetings of the Release team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants