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

src: add NODE_MODULE_VERSION registry #24114

Closed

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 6, 2018

Ref: https://nodejs.org/en/download/releases/
Ref: https://github.com/lgeiger/node-abi/blob/master/index.js
Ref: nodejs/TSC#621

As per nodejs/TSC#621, here's a suggestion for how we keep track of these values as they spiral out of control. Already the comments in node_version.h is out of date and the suggested means of "reserving" a value by opening an issue at nodejs/TSC is going to be difficult to track.

I've collected the values as best as I can. Partly from our own history but also @lgeiger's node-abi package and a bit of digging. I couldn't figure out how NW.js stores NODE_MODULE_VERSION so can't update their values (maybe @rogerwang can help?).

Versions are all node-semver range compliant. The V8 increments are pinned to the precise pre-release version they'd compile as if you did it raw (won't match nightly versions unfortunately).

Thoughts?

/cc @nodejs/addon-api @nodejs/v8 @kapouer

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Nov 6, 2018
@nicolasnoble
Copy link
Contributor

Having some data structure intersection with https://nodejs.org/dist/index.json might be a good idea in order to make parsing a bit less cumbersome?

@rvagg
Copy link
Member Author

rvagg commented Nov 6, 2018

Any suggestions on that @nicolasnoble? index.json has NODE_MODULE_VERSION in it already as "modules", are you suggesting just a rename here?

Cross-referencing the two files would be a matter of filtering by "runtime" and "variant" and then checking semver range compatibility. But since "modules" is already in index.json I'm not sure the utility of doing that. This is more helpful for reservation purposes but could also be easily consumed by prebuild tooling.

@nicolasnoble
Copy link
Contributor

Yes, I'm basically just suggesting a rename so that the same kind of tooling can be used to parse both files :-)

@rvagg
Copy link
Member Author

rvagg commented Nov 6, 2018

fair call, not sure it's self documenting enough in here though, it certainly looks more pleasant and compact:

screenshot 2018-11-06 20 23 21

screenshot 2018-11-06 20 23 36

Latest commit has the latter, previous commit has the former. Anyone else have input?

@Flarna
Copy link
Member

Flarna commented Nov 6, 2018

What I miss here is some more information about where a specific variant is maintained to get the full diff.

Maybe the names used in variant field could be more explicit or include some URL to clearly point to the "owner" of the version number.

@rvagg
Copy link
Member Author

rvagg commented Nov 6, 2018

@Flarna I would hope that over time the commit log would begin to show who owns what, if you're reserving a number then you're likely going to have a commit to do it. Do we need to surface additional metadata in the file itself? What would be the use-case for that?

@Flarna
Copy link
Member

Flarna commented Nov 7, 2018

As a native addon developer I have to decide if I want to support these variants. A change in module version can be any sort of change (not just ABI) and may need also adaptions in other modules like NAN.

I'm not sure if we can expect that variant vendors take care to adapt also NAN so this will be left as exercise for addon developers. To actually do this adaptions it's needed to know all details of the difference.

But not all variants are of interest for everyone.

There are several criterias like e.g.:

  • purpose of the variant: electron differs in usecase to node so at lot addons may decide to skip support for it as it is not applicable
  • number of users of the variant: e.g. -pre versions of node are hopefully not used in production so skipping them is likely valid for a lot users
  • debian users usually don't want closed source/precompiled dependencies ==> no need to maintain a per-compiled binary for the debian variants.

@rogerwang
Copy link
Member

Thanks for CC @rvagg . NW.js keeps tracks of the Node version it used in https://nwjs.io/versions.json . I can certainly help update the registry file in Node.

@rvagg
Copy link
Member Author

rvagg commented Nov 7, 2018

@Flarna right, so this is mostly about making this more informative for addon authors. I get that, but I'm wondering if that's best achieved in this file or maybe in a matching .md file that can be more freeform. I'll have a play with extending this file and see if I can keep mess to a minimum in the process.

@rvagg
Copy link
Member Author

rvagg commented Nov 7, 2018

@rogerwang I might ping you again if/when this lands so you can PR an update. I'm not sure if I can make a 1:1 matching of Node version to NODE_MODULE_VERSION since you're also using -pre versions. So it might be best if you do a separate PR against the file to get those versions recorded properly.

@Flarna
Copy link
Member

Flarna commented Nov 7, 2018

@rvagg I don't care much about the format here so moving the details into an .md file is also fine - as long as they stay in sync.

@rvagg
Copy link
Member Author

rvagg commented Nov 7, 2018

it's the sync problems I'm most concerned about, already node_version.h is out of sync and it's all in one file!

src/node_version.h Outdated Show resolved Hide resolved
@rvagg
Copy link
Member Author

rvagg commented Jan 30, 2019

Updated this to reserve 68 which is in use now for nightlies

  { "modules": 68, "runtime": "node",     "variant": "v8_7.1",               "versions": "12.0.0-pre" },

TODO here is:

  • Consider some future-proofing given that we may be adjusting approach in the future, possibly by letting embedders use a uniqie higher-order bit so they can increment independently - we'll want a record of who is using what value in a registry so we can hand those out uniquely, plus a registry of values in use will still be useful for prebuild tools so we'll want to encourage them to report values back to us. We may also have something like POC: src: add addon ABI declaration option #25539 in use and we need to consider if that has any impact on a registry, it may or may not make sense to have some of that information included in here (perhaps just what _ABI_ numbers correspond to what dependencies?)
  • Consider an expansion to the "runtime" and/or "variant" that provides more information for addon authors as per @Flarna's comments. I'm thinking that a separate top-level object in here, "variants" or something like that, with at least some descriptive text to explain what it is, who maintains it, a url, maybe even something machine readable to make easy decisions for prebuilding ("release": false?).

I think the obvious step in getting there for now is to just bump this whole structure down one level so more information can be introduced at the top level and this information could potentially be archived if we ever arrived at a full replacement. Something like this:

screenshot 2019-01-31 09 42 16

@rvagg
Copy link
Member Author

rvagg commented Mar 12, 2019

added 69 for Electron 4 and 70 for Electron 5 per discussion in nodejs/TSC#651

@rvagg
Copy link
Member Author

rvagg commented Mar 17, 2019

This is ready to land (IMO), could I get some reviews please? If you disagree with what's in this PR please make that clear.

Latest changes:

  • Updated for V8 7.4 which is expected to be the Node 12 version (72)
  • Renamed to doc/abi_version_registry.json
  • Turned the JSON structure into an object and embedded the modules version number array into a "NODE_MODULE_VERSION" property

So this is now designed to be extended into the future, it'll support:

  • Adding further metadata, perhaps URLs and contact details for owners of different "runtime"s and/or "variant"s (I don't think that's necessary to get this landed and am not convinced it's needed at all but perhaps someone else is)
  • New versions of ABI matching that may come from future work, such as the match-per-dependency-version approach that @ofrobots has been outlining in NODE_MODULE_VERSION for Electron Major Releases TSC#651

@nicolasnoble
Copy link
Contributor

Electron 4.0.4 changed its NVM from 64 to 69. There has been debate over if this constituted an ABI breaking change or a fix, and if this was relevant in a patch release or in a major release. See the mentions in electron/electron#16687 and the discussion in electron/node@8bc5d17

It might be worth discussing this here also in order to provide clarification and guidance in the documentation.

@rvagg
Copy link
Member Author

rvagg commented Mar 17, 2019

@nicolasnoble it did come up in nodejs/TSC#651 and the reservation of 69 was as a result of that discussion. It's less than ideal for addon authors and consumers in Electron but Electron has additional problems with ABI stability that it might need to monitor more carefully into the future. Having this as a place to register as many new numbers as they need should help resolve concerns about bumping more liberally.

I've made changes to be more specific about Electron versions (5da2462):

-    { "modules": 69, "runtime": "electron", "variant": "electron",             "versions": "4" },
+    { "modules": 69, "runtime": "electron", "variant": "electron",             "versions": "^4.0.5" },

and

-    { "modules": 64, "runtime": "electron", "variant": "electron",             "versions": "3" },
+    { "modules": 64, "runtime": "electron", "variant": "electron",             "versions": ">=3 <4.0.5" },

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

Thanks, @rvagg!

@nicolasnoble
Copy link
Contributor

The change happened between 4.0.3 and 4.0.4, so your patch here is a little bit off.

doc/releases.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
BUILDING.md Outdated Show resolved Hide resolved
@vsemozhetbyt vsemozhetbyt added the embedding Issues and PRs related to embedding Node.js in another project. label Mar 17, 2019
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM once @vsemozhetbyt comments are added.

BUILDING.md Outdated Show resolved Hide resolved
@murgatroid99
Copy link
Contributor

Can you state in the BUILDING file that changing the NODE_MODULE_VERSION in a Node distribution is an ABI breaking change and should therefore be accompanied by a major version bump because the number determines whether the runtime will load an addon?

In other words, a module built for NODE_MODULE_VERSION X can not be loaded by a Node runtime with NODE_MODULE_VERSION Y, so changing the NODE_MODULE_VERSION in the runtime from X to Y breaks compatibility with installed addons.

@rvagg
Copy link
Member Author

rvagg commented Apr 16, 2019

Suggestions all adopted, I think this is ready for landing and I'll do that tomorrow unless there objections.

@murgatroid99 to your point: that's covered fairly comprehensively in doc/releases.md under the heading "Also consider whether to bump NODE_MODULE_VERSION". Beyond the releases we make here the best we can do is help coordinate NODE_MODULE_VERSIONs. What embedders do with their version numbers is out of our hands and how the addon ecosystem handles that is also not something we have much control over—but having a registry of these numbers is certainly going to make it easier!

@rvagg rvagg force-pushed the rvagg/node_module_version_registry branch from 5da2462 to 72aaacc Compare April 16, 2019 07:44
@rvagg
Copy link
Member Author

rvagg commented Apr 17, 2019

Landed in c61c722

@rvagg rvagg closed this Apr 17, 2019
@rvagg rvagg deleted the rvagg/node_module_version_registry branch April 17, 2019 10:28
rvagg added a commit that referenced this pull request Apr 17, 2019
PR-URL: #24114
Refs: https://nodejs.org/en/download/releases/
Refs: https://github.com/lgeiger/node-abi/blob/master/index.js
Refs: nodejs/TSC#621
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
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. embedding Issues and PRs related to embedding Node.js in another project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.