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 note about ABI compatibility #22237

Closed
wants to merge 1 commit into from

Conversation

@MylesBorins
Copy link
Member

commented Aug 10, 2018

Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

@richardlau

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

release. To maintain ABI compatibility it is expected that production builds of
Node.js will be built against the same version of dependencies as the project
vendors. If Node.js is to be built against a different version of a dependency
please create a custom `NODE_MODULE_VERSION` to ensure ecosystem compatibility.

This comment has been minimized.

Copy link
@addaleax

addaleax Aug 10, 2018

Member

We should probably strongly encourage people who do this to contact us, so that we have consistent NODE_MODULE_VERSION values between distributors?

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Aug 10, 2018

Author Member

So qq, I wasn't sure the answer to, can we support NODE_MODULE_VERSION to have characters in it or does gyp assume it is a number?

This comment has been minimized.

Copy link
@jasnell

jasnell Aug 10, 2018

Member

I'm curious if this is something we could actually detect during build so that the build can actually fail if the NODE_MODULE_VERSION is not bumped.

This comment has been minimized.

Copy link
@addaleax

addaleax Aug 10, 2018

Member

@MylesBorins The value is programatically accessible to addons, so changing its type would be a breaking change by itself.

@jasnell We might be able detect mismatching dependency versions, but not every API version change also means that the ABI has changed (or vice versa)

This comment has been minimized.

Copy link
@richardlau

richardlau Aug 10, 2018

Member

I guess the implication of this is we'll need to maintain a list of NODE_MODULE_VERSION. Do we have anything other than

node/src/node_version.h

Lines 74 to 114 in dcfd323

/**
* Node.js will refuse to load modules that weren't compiled against its own
* module ABI number, exposed as the process.versions.modules property.
*
* When this version number is changed, node.js will refuse
* to load older modules. This should be done whenever
* an API is broken in the C++ side, including in v8 or
* other dependencies.
*
* Node.js will not change the module version during a Major release line
* We will at times update the version of V8 shipped in the release line
* if it can be made ABI compatible with the previous version.
*
* Module version by Node.js version:
* Node.js v0.10.x: 11
* Node.js v0.12.x: 14
* Node.js v4.x: 46
* Node.js v5.x: 47
* Node.js v6.x: 48
* Node.js v7.x: 51
* Node.js v8.x: 57
*
* Module version by V8 ABI version:
* V8 5.4: 51
* V8 5.5: 52
* V8 5.6: 53
* V8 5.7: 54
* V8 5.8: 55
* V8 5.9: 56
* V8 6.0: 57
* V8 6.1: 58
* V8 6.2: 59
* V8 6.3: 60
* V8 6.4: 61
* V8 6.5: 62
* V8 6.6: 63
* V8 6.7: 64
* V8 6.8: 65
*
* More information can be found at https://nodejs.org/en/download/releases/
*/
?

@ArchangeGabriel

This comment has been minimized.

Copy link

commented Aug 10, 2018

@MylesBorins Likely not the right place, this file seems to be targeted at NodeJS upstream maintainer responsible for doing releases.

Maybe https://github.com/nodejs/node/blob/master/BUILDING.md would be a better place?

Also I would then advocate to specify in each release which version of each library was used for this release. Those are information that can be retrieved by looking at the commit history of the deps/ folder, but having it directly accessible from there would be a plus. ;)

@@ -186,6 +186,14 @@ informed perspective, such as a member of the NAN team.
see a need to bump `NODE_MODULE_VERSION` then you should consult the TSC.
Commits may need to be reverted or a major version bump may need to happen.

*Note*: The Node.js ecosystem is reliant on ABI compatibility within a Semver

This comment has been minimized.

Copy link
@Trott

Trott Aug 10, 2018

Member

Micro-nit: Remove *Note*: here and in the paragraph above. Just say what you have to say.

@@ -186,6 +186,14 @@ informed perspective, such as a member of the NAN team.
see a need to bump `NODE_MODULE_VERSION` then you should consult the TSC.
Commits may need to be reverted or a major version bump may need to happen.

*Note*: The Node.js ecosystem is reliant on ABI compatibility within a Semver
Major release. To maintain ABI compatibility it is expected that production

This comment has been minimized.

Copy link
@Trott

Trott Aug 10, 2018

Member

Nit: Semver Major -> semver major for consistency.

I dislike the term semver major because I think it's jargon that we understand but that not everyone should be expected to. However, I don't have a better alternative. Suggestions welcome, although it's certainly outside the scope of this PR. It's been bugging me for a long time.

This comment has been minimized.

Copy link
@addaleax

addaleax Aug 10, 2018

Member

I think just within a major version is fine?

@vsemozhetbyt vsemozhetbyt added the build label Aug 10, 2018

@maclover7
Copy link
Member

left a comment

LGTM with nits addressed

@richardlau

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

Also I would then advocate to specify in each release which version of each library was used for this release. Those are information that can be retrieved by looking at the commit history of the deps/ folder, but having it directly accessible from there would be a plus. ;)

For stuff that we've built and released on https://nodejs.org, the versions of the dependencies are recorded in https://nodejs.org/dist/index.json and https://nodejs.org/dist/index.tab.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2018

I've addressed all above nits and moved the note into building.md

Thoughts?

BUILDING.md Outdated
builds of Node.js will be built against the same version of dependencies as the
project vendors. If Node.js is to be built against a different version of a
dependency please create a custom `NODE_MODULE_VERSION` to ensure ecosystem
compatibility. Please consult with the TSC if you decide to create a custom

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Aug 10, 2018

Member

consult with the TSC may seem a bit vague to someone unfamiliar with our organization. Maybe explicitly suggest them to open an issue in https://github.com/nodejs/tsc/issues ? (Also it's likely to be request to join @nodejs/delivery-channels)

This comment has been minimized.

Copy link
@mhdawson

mhdawson Aug 10, 2018

Member

I think in line with the above suggestion for reversed bits we could ask that the PR themselves into the list of reserved values for those bits.

release. To maintain ABI compatibility it is expected that production
builds of Node.js will be built against the same version of dependencies as the
project vendors. If Node.js is to be built against a different version of a
dependency please create a custom `NODE_MODULE_VERSION` to ensure ecosystem

This comment has been minimized.

Copy link
@ofrobots

ofrobots Aug 10, 2018

Contributor

I think we should flesh out a bit more what is involved in a custom NODE_MODULE_VERSION. The problem is that we don't necessarily have "room" in the incrementally increasing versions to anticipate needs apriori.

My proposal is:

  • Reserve 8 most significant bits in the NODE_MODULE_VERSION for 'distribution'.
  • Official builds would use 0. Electron, and other distributors could use a different value if the binary they are shipping is not ABI compatible.

This comment has been minimized.

Copy link
@mhdawson

mhdawson Aug 10, 2018

Member

reserving a number of bits makes sense to me.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Aug 10, 2018

Author Member

+1. Does someone want to push a commit that adds this specific documentation?

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 2, 2018

Author Member

@ofrobots would you be able to supply explicit copy for this? Otherwise I'd like to land this in an iteration of this PR

@nornagon

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

just chiming in to say this would be pretty useful for electron. right now people get confused and think that their module should work because it has the right NODE_MODULE_VERSION, but in fact it was built against a non-Electron set of headers. It'd be great if we could use a different NMV to make it really clear that building against stock headers won't work.

@eli-schwartz

This comment has been minimized.

Copy link

commented Aug 13, 2018

This seems a bit vague to me, you're essentially saying "any dependency needs to be the same version, no matter what", but I only understand exactly one dependency to actually be public ABI, that being openssl due to exporting openssl symbols to binary node_modules.

Could this be a little more specific so that distro packagers can actually know what they're dealing with? For example we build with icu 62, even on nodejs-lts-carbon which vendors icu 60. Is this too, to be considered public ABI?

e.g.:

The Node.js ecosystem is reliant on ABI compatibility for binary modules within a major release. To maintain ABI compatibility it is required that production builds of Node.js will be built against the same version of publicly exported dependencies as the project vendors. If Node.js is to be built against a different version of a dependency then in order to ensure ecosystem compatibility and prevent breaking ABI, it is necessary to create a custom NODE_MODULE_VERSION. Please consult with the TSC if you decide to create a custom NODE_MODULE_VERSION so we can avoid duplication in the ecosystem.

Currently, Node.js exports the following dependencies as part of its public ABI:

  • openssl

P.S. This is a statement being made about whether or not ABI compatibility is maintained, and this does, apparently, require the same versions of dependencies. Stating that it is "expected" is a rather soft way of describing a technological requirement that has no ambiguity.

Either there is ABI compatibility or there is not. Saying that you don't expect ABI compatibility implies you think there is a slim chance that there might accidentally be ABI compatibility anyway.

@mscdex mscdex referenced this pull request Aug 24, 2018
3 of 3 tasks complete
@trivikr

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

Ping

@jasnell
jasnell approved these changes Oct 1, 2018
@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

I need to review this and update based on comments. Will try and do so later today. If anyone else wants to take lead and push a commit please feel free to do so

@MylesBorins MylesBorins force-pushed the MylesBorins:abi-compat-note branch from f9dd79c to e47d5c6 Oct 2, 2018

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

updated, will land in 48 hours if there are no objections

@trivikr
trivikr approved these changes Oct 2, 2018
@eli-schwartz

This comment has been minimized.

Copy link

commented Oct 3, 2018

The technical connotations i mentioned of expected vs. required have been clarified sufficiently IMO.

You still don't enumerate what the publicly exported dependencies in question are, though. Are there any, other than openssl?

@MylesBorins MylesBorins force-pushed the MylesBorins:abi-compat-note branch from e47d5c6 to c22f25b Oct 3, 2018

@JCMais
JCMais approved these changes Oct 4, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

@MylesBorins MylesBorins force-pushed the MylesBorins:abi-compat-note branch from c22f25b to 97b6139 Oct 29, 2018

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

/cc @nodejs/platform-aix is this a known failure? I'm going to land this anyways as it is a doc fix unrelated to the failures but wanted to raise this

@MylesBorins

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

Landed in 97d9ccd

MylesBorins added a commit that referenced this pull request Oct 29, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@richardlau richardlau referenced this pull request Oct 29, 2018
@richardlau

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

/cc @nodejs/platform-aix is this a known failure? I'm going to land this anyways as it is a doc fix unrelated to the failures but wanted to raise this

I don't believe it's a known failure. I've opened #23962 to look into it.

targos added a commit that referenced this pull request Nov 1, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@targos targos referenced this pull request Nov 1, 2018
MylesBorins added a commit that referenced this pull request Nov 26, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 26, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebytere codebytere referenced this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
rvagg added a commit that referenced this pull request Nov 28, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
doc: add note about ABI compatibility
Building node against versions of the dependencies that differ from the
ones we vendor will result in a non ABI compatible version of Node.js

This patch adds a note to make it explicit that if individuals build
node against different versions of a dependency they should make a
custom NODE_MODULE_VERSION.

PR-URL: #22237
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebytere codebytere referenced this pull request Nov 29, 2018
@BethGriggs BethGriggs referenced this pull request Dec 4, 2018
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.