doc: add note about ABI compatibility

[MylesBorins]

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.

[nodejs-github-bot]

@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

[richardlau]

cc @nodejs/delivery-channels

[addaleax]

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

[MylesBorins]

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?

[jasnell]

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.

[addaleax]

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

[richardlau]

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]

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

[Trott]

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

[Trott]

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.

[addaleax]

I think just within a major version is fine?

[maclover7]

LGTM with nits addressed

[richardlau]

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]

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

Thoughts?

[joyeecheung]

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)

[mhdawson]

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.

[ofrobots]

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.

[mhdawson]

reserving a number of bits makes sense to me.

[MylesBorins]

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

[MylesBorins]

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

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

[trivikr]

Ping

[MylesBorins]

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]

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

[eli-schwartz]

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]

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

[MylesBorins]

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

Landed in 97d9ccd

[richardlau]

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