doc: add note about ABI compatibility by MylesBorins · Pull Request #22237 · nodejs/node · GitHub

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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/493/pipeline

cc @nodejs/delivery-channels

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

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?

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.

@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)

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/ 
        
            */

?

@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. ;)

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

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.

I think just within a major version is fine?

LGTM with nits addressed

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.

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

Thoughts?

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)

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.

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.

reserving a number of bits makes sense to me.

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

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

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.

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.

Ping

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

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

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?

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.

CI: https://ci.nodejs.org/job/node-test-pull-request/18194/

/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

Landed in 97d9ccd

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>

/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.

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>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 10, 2018

MylesBorins mentioned this pull request Aug 10, 2018

NodeJS native modules made difficult by distribution packages. #21897

Closed

addaleax reviewed Aug 10, 2018

View reviewed changes

Trott reviewed Aug 10, 2018

View reviewed changes

vsemozhetbyt approved these changes Aug 10, 2018

View reviewed changes

vsemozhetbyt added the build Issues and PRs related to build files or the CI. label Aug 10, 2018

maclover7 approved these changes Aug 10, 2018

View reviewed changes

joyeecheung reviewed Aug 10, 2018

View reviewed changes

ofrobots reviewed Aug 10, 2018

View reviewed changes

benjamingr approved these changes Aug 10, 2018

View reviewed changes

mscdex mentioned this pull request Aug 24, 2018

doc: add blurb about implications of ABI stability #22508

Closed

3 tasks

jasnell approved these changes Oct 1, 2018

View reviewed changes

MylesBorins force-pushed the abi-compat-note branch from f9dd79c to e47d5c6 Compare October 2, 2018 13:12

trivikr approved these changes Oct 2, 2018

View reviewed changes

MylesBorins force-pushed the abi-compat-note branch from e47d5c6 to c22f25b Compare October 3, 2018 02:55

JCMais approved these changes Oct 4, 2018

View reviewed changes

MylesBorins force-pushed the abi-compat-note branch from c22f25b to 97b6139 Compare October 29, 2018 04:38

MylesBorins closed this Oct 29, 2018

richardlau mentioned this pull request Oct 29, 2018

AIX build failure #23962

Closed

targos mentioned this pull request Nov 1, 2018

Release proposal: v11.1.0 #23922

Merged

This was referenced Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#245

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#246

Closed

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#247

Closed

eli-schwartz mentioned this pull request Nov 2, 2018

NODE_MODULE_VERSION debian/sid 8 and 10 nodejs/TSC#621

Closed

beevelop mentioned this pull request Nov 2, 2018

2018-11-02 Version 11.1.0 (Current) @targos beevelop/docker-android-nodejs#248

Closed

MylesBorins added land-on-v8.x labels Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

BethGriggs mentioned this pull request Dec 4, 2018

v8.14.1 proposal #24832

Merged

	/**
	* 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/
	*/

doc: add note about ABI compatibility by MylesBorins · Pull Request #22237 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: add note about ABI compatibility - #22237

doc: add note about ABI compatibility#22237MylesBorins wants to merge 1 commit intonodejs:masternodejs/node:masterfrom MylesBorins:abi-compat-noteMylesBorins/node:abi-compat-noteCopy head branch name to clipboard

Conversation

MylesBorins commented Aug 10, 2018

Uh oh!

nodejs-github-bot commented Aug 10, 2018

Uh oh!

richardlau commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArchangeGabriel commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Aug 10, 2018

Uh oh!

MylesBorins commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofrobots Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nornagon commented Aug 10, 2018

Uh oh!

eli-schwartz commented Aug 13, 2018

Uh oh!

trivikr commented Oct 1, 2018

Uh oh!

MylesBorins commented Oct 1, 2018

Uh oh!

MylesBorins commented Oct 2, 2018

Uh oh!

eli-schwartz commented Oct 3, 2018

Uh oh!

MylesBorins commented Oct 29, 2018

Uh oh!

MylesBorins commented Oct 29, 2018

Uh oh!

MylesBorins commented Oct 29, 2018

Uh oh!

richardlau commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

doc: add note about ABI compatibility#22237
MylesBorins wants to merge 1 commit into
nodejs:masterfrom
MylesBorins:abi-compat-note

ofrobots Aug 10, 2018 •
edited
Loading