-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
doc: add guide about abi stability #23229
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
Conversation
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might start with "An Application Binary Interface (ABI) is a way for programs to call other functions and use data structures from other compiled programs (binaries)" or something like that?
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ABI provider ('the library being required') and ABI user ('the consuming code')
in the document maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say "the library being required", because in the case of a native addon, it's actually the executable program that is being required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wouldn't say "consuming" either, because that's too abstract. I can change it to "provider of the ABI" and "user of the ABI" to make it more down-to-Earth.
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, this means that Node.js commits that a native addon compiled against...
?
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiple major Node.js versions ?
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be painfully obvious: ...and remains backwards compatible.
?
doc/guides/abi-stability.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would link to the N-API and native-addon docs here perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean https://nodejs.org/docs/latest/api/addons.html and https://nodejs.org/docs/latest/api/n-api.html? I don't quite understand why I would link to the docs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very +1 on adding this guide overall and it contains good content - left some nits -I think the start could be more new-user friendly in terms of motivating ABI.
d07bb3c
to
09cd67c
Compare
@benjamingr I addressed your review comments, and I have questions about some of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Landed in 65366ad. |
Re: nodejs/abi-stable-node#332 (comment) PR-URL: nodejs#23229 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Re: nodejs/abi-stable-node#332 (comment) PR-URL: #23229 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Re: nodejs/abi-stable-node#332 (comment) PR-URL: #23229 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Re: nodejs/abi-stable-node#332 (comment) PR-URL: #23229 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Re: nodejs/abi-stable-node#332 (comment)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes