-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
doc: add guide about abi stability #23229
Conversation
doc/guides/abi-stability.md
Outdated
# ABI Stability | ||
|
||
## Introduction | ||
An Application Binary Interface (ABI) is the compiled version of an Application |
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
agree with those with which the ABI provider was compiled. This is usually | ||
accomplished by compiling against the headers provided by the ABI provider. | ||
|
||
Since the ABI provider and the ABI user may be compiled at different times with |
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
the Node.js project has adopted [semantic versioning](https://semver.org/). | ||
This ensures that the APIs provided by the project will result in a stable ABI | ||
for all minor and patch versions of Node.js released within one major version. | ||
In practice, this means that a Node.js native addon compiled against a given |
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
|
||
## N-API | ||
Demand has arisen for equipping Node.js with an API that results in an ABI that | ||
remains stable across multiple Node.js versions. The motivation for creating |
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
|
||
N-API is versioned because new APIs are added from time to time. Unlike | ||
semantic versioning, N-API versioning is cumulative. That is, each version of | ||
N-API conveys the same meaning as a minor version in the semver system. |
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.
?
existing users of the ABI incompatible with the new version. | ||
|
||
## ABI Stability in Node.js | ||
Node.js provides header files maintained by several independent teams. For |
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