★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22508Nouvelle recherche | Portail wanayoo
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

doc: add blurb about implications of ABI stability #22508

Conversation

Projects
None yet
8 participants
@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Aug 24, 2018

Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

Although N-API provides an ABI stability guarantee, other parts of Node.js do
not, and any external libraries used from the addon may not. In particular,
neither of the following Node.js APIs provides an ABI stability guarantee:

This comment has been minimized.

@addaleax

addaleax Aug 24, 2018

Member

I think this wording is a bit misleading – we do have ABI stability guarantees, but we follow semver rather than not allowing any breakage.

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Aug 24, 2018

Author Contributor

Good point - sorry!

This comment has been minimized.

@mscdex

mscdex Aug 24, 2018

Contributor

How about instead (s/neither/none/, s/Node.js//, s/provides/provide/):

In particular, none of the following APIs provide an ABI stability guarantee:

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Aug 24, 2018

Perhaps this could be combined with #22237 or vice versa?

#include <node_object_wrap.h>
#include <node_perf_common.h>
#include <node_platform.h>
#include <node_version.h>

This comment has been minimized.

@addaleax

addaleax Aug 24, 2018

Member

Only node_buffer.h, node_version.h, node_object_wrap.h and node.h (+ N-API) are real public API. I’ll open a PR to put the others behind NODE_WANT_INTERNALS.

@@ -367,6 +367,10 @@ set of APIs that are used by the native code. Instead of using the V8
or [Native Abstractions for Node.js][] APIs, the functions available
in the N-API are used.

Creating and maintaining an add-on that benefits from the ABI stability

This comment has been minimized.

@Trott

Trott Aug 25, 2018

Member

add-on -> addon for consistency with the rest of the doc.

This comment has been minimized.

@Trott

Trott Aug 25, 2018

Member

Actually, I'll just go ahead and make that change myself since I'm right here...

@lpinca

lpinca approved these changes Aug 25, 2018

Copy link
Member

lpinca left a comment

LGTM with comments addressed.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Aug 27, 2018

@mscdex I believe there's a difference between this ABI compatibility note and the one in #22237, in that herein we deal with ABI compatibility across major versions of Node.js, whereas in the other we deal with ABI compatibility within a major version of Node.js.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Aug 28, 2018

@addaleax I have updated the text to reflect your comments.

@mhdawson
Copy link
Member

mhdawson left a comment

LGTM - once linter is green...

@BridgeAR

This comment has been minimized.

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Aug 31, 2018

```C
#include <node_api.h>
```
and by checking, for all external library that it uses, that the external

This comment has been minimized.

@addaleax

addaleax Sep 2, 2018

Member

typo: libraries

@gabrielschulhof gabrielschulhof force-pushed the gabrielschulhof:abi-stability-implications branch from d331e95 to 7ba8ae4 Sep 3, 2018

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Sep 3, 2018

@gabrielschulhof

This comment has been minimized.

Copy link
Contributor Author

gabrielschulhof commented Sep 3, 2018

Landed in 7033fc7.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Sep 3, 2018

doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

PR-URL: nodejs#22508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@gabrielschulhof gabrielschulhof deleted the gabrielschulhof:abi-stability-implications branch Sep 3, 2018

targos added a commit that referenced this pull request Sep 3, 2018

doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

PR-URL: #22508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

targos added a commit that referenced this pull request Sep 3, 2018

doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

PR-URL: #22508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@targos targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

targos added a commit that referenced this pull request Sep 6, 2018

doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

PR-URL: #22508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

doc: add blurb about implications of ABI stability
Mention that ABI stability can be achieved only by linking to ABI-
stable parts of Node.js and to other libraries which are ABI-stable.

PR-URL: nodejs#22508
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.