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

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 nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 10, 2018
@richardlau
Copy link
Member

cc @nodejs/delivery-channels

doc/releases.md Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

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

doc/releases.md Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/releases.md Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just within a major version is fine?

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

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits addressed

@richardlau
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@ofrobots ofrobots Aug 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reserving a number of bits makes sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

trivikr commented Oct 1, 2018

Ping

@MylesBorins
Copy link
Member Author

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
Copy link
Member Author

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

@eli-schwartz
Copy link
Contributor

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.
@MylesBorins
Copy link
Member Author

@MylesBorins
Copy link
Member Author

/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
Copy link
Member Author

Landed in 97d9ccd

MylesBorins added a commit that referenced this pull request Oct 29, 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.

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 mentioned this pull request Oct 29, 2018
@richardlau
Copy link
Member

/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 pushed a commit that referenced this pull request Nov 1, 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.

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
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
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 mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 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.

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 pushed a commit that referenced this pull request Nov 28, 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.

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
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 mentioned this pull request Nov 29, 2018
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet