NODE_MODULE_VERSION for Electron Major Releases

@MarshallOfSound

Related to #621
See Also: electron/node-abi#55

At present Electron has the same ABI number for Electron 3 and Electron 4, when in reality the ABI changed between them and so this is causing issues for some consumers.

Would it be possible for us to get our own ABI versions for each major Electron release?

cc @MarshallOfSound

Given that Electron is an embedder of Node.js, what is currently blocking you to do this?

Will it have any downsides?

@ofrobots

I think there has been some earlier discussion along these lines. I don't think Node.js would do any releases etc. instead what would be needed is so that the format of the ABI number be structured so that embedders would add their own versions without overlapping each other of further Node.js versions. I think @ofrobots had suggested possibly assigning some of the higher bits for this purpose.

@ofrobots

I see the issue above #621 is where @ofrobots made his suggestion :). I also see that @rvagg had an alternate approach where there would be a registry with a way to reserve numbers.

I guess the action being requested is for us (Node.js) to agree/decide on one of these approaches.

@rvagg

I guess the action being requested is for us (Node.js) to agree/decide on one of these approaches.

Pretty much 😄

We've been following along with these issues and the PR that @rvagg raised but now this has actually manifested as an issue with Electron 3 vs 4 we need to get a unique ABI number. Either the higher-order-bit, an allocated range, or laying claim to a number in a registry works for us (although the higher-order-bit or an allocated range would be easier for us to manage). Just need Node.JS to say "do this" 😀

Is it too early to put a tsc-agenda label on this just to get a decision made?

I'd say add it to the agenda so we can discuss and get agreement (if we can) on which of the two directions is preferred. Bits or registry. I'll add, if people disagree please let me know and we can remove.

I'm concerned about #621 (comment) (explosion of binaries with different versions numbers for pre-compiled addon providers to maintain). There are also two issues here, AFAICT:

For maintainers to know which Node.js version ships which ABIs, and
For the addon to advertise which (combination of) Node.js ABIs it supports.

I believe that, if we are to avoid the combinatorial explosion resulting from assigning version numbers to each ABI Node.js exposes, we need more than just vendor plus Node.js ABI version in the nm_version field. We also need a way for the field to indicate that the module does not care about, say, the openssl ABI version. So, we need to further partition the bits, and have a zero in a given field indicate that the module does not care about that particular ABI. This would be true for all ABI-related fields except the highest order one, which would be the vendor field, where a zero would indicate that the module is designed to run against vanilla Node.js.

The registry approach, although AFAICT more open-ended, I don't think allows modules to indicate which ABIs they don't care about.

Do we have a list of ABIs whose version would influence the final version number advertised by the addon? I'm thinking, like,

ABI
vendor
V8
openssl
libuv
ICU(?)
c-ares(?)

We can also make struct node::node_module a variable-length structure where the fields currently present would be followed by a new field indicating how many ABI versions follow and then a sequence of ABI versions. A value of -2 in nm_version (-1 is taken by N-API) would then indicate to the addon loader that it needs to consult the ABI versions in detail.

How is this could affect prebuilt libraries? Could an author load a different prebuilt based on this composed ABI?

@mcollina

@mcollina the prebuild creation tools like node-pre-gyp and prebuild would have to be updated to deal with all these new versions that Node.js exposes and encode them in the file names that get uploaded, but, if authors do not care about most of the ABIs they would still only need to build a few binaries for the combinations they care about.

There's another aspect too. We are trying to move addons towards using a special symbol for startup rather than node::node_addon + a library constructor, because library constructors only get called once per process, so such addons would not support workers and/or multiple contexts.

Currently the special symbol's name includes the module version. It's declared, like,

void node_register_module_v64(v8::Local<v8::Object> exports,
                              v8::Local<v8::Object> module,
                              v8::Local<v8::Context> context);

with the help of src/node.h#L540-L560

So, if we want such an addon to first report the ABI versions it supports, we might want to make this a two-step process where the special symbol is a function that returns the list of ABIs supported and the entry point for the addon.

@ofrobots

I'm personally interested in @ofrobots initial suggest of embedders being able to register a most significant bit for their version. This scales to n number of releases and avoids us having to increment the version number every time new platforms need to bump (which I don't see as scalable)

@Trott

@Trott thanks, missed that part when I removed it from the agenda.

@codebytere

So we've got quite side-tracked here from the original request, for good reason obviously, but we need to get Electron unblocked according to the current state of play.

We discussed this in our TSC meeting today (private section sorry, there was nothing sensitive about the discussion, I just forgot to raise it while we were recording). And came up with a rough plan for moving forward. First though we need to unblock this issue.

@codebytere asked:

Would it be possible for us to get our own ABI versions for each major Electron release?

The answer is: yes, you can have as many as you need. But for right now, the process requires some coordination so let's reserve the numbers you need.

If Electron would like one, right now 69 is available. I have the latest list updated in nodejs/node#24114 and if you want 69 I can add that in and we'll make sure we don't use it ourselves. Let us know here and we can consider this issue resolved.

For now, the process as documented is still to open an issue here to request a new number and we'll take care of the rest, as in #621. But that will likely change, because it's leading to discussions like this and slow resolution to something that should be simple!

So, on to the other matters discussed here.

The idea of handing out higher order bits so you can move independently is on the table, but even if we do that, a central registry is going to be helpful for coordinating amongst prebuild tools that manage binaries so we'll probably want some central reporting mechanism—it just won't need to be used as a mutex and can take reporting after the fact. We're also going to end up with awkward strings which addon authors are going to have to use (i.e. they currently put the modules number in the filename of binaries (e.g)), not a blocker, just likely confusing. 16777285 or 0x1000045 if we gave Electron an incremented highest order bit for 32-bit integers for example (I think). @ofrobots are you seeing a slightly more elegant version than this? We should probably check with prebuild tool authors on this when we have a clearer proposal and perhaps we should come up with a proposal specifically for the Electron case first since many addons are already publishing binaries for it.

There's also @gabrielschulhof's nodejs/node#25539, which is additive rather than a replacement to anything we're talking about. A registry, or at least record of values behind some of the _ABI_ numbers may also be useful rather than just tracking increments in our header files, so I've made a note in my registry PR nodejs/node#24114 that being able to accommodate such data in the future should be considered.

The goals and facts below are reflected to some extent in my PR, but I think we should think about them first before we think about any implementation.

Goals:

An addon loads exactly when its ABI matches and fails with a thrown exception otherwise. Bonus: The exception reveals where in the ABI the mismatch was found.
Addon maintainers need to build as few binaries as possible.
Addon maintainers need to run their build as rarely as possible. Ideally they never need to bump their version because a new version of Node.js appears bearing a new version of an ABI that they don't use in their addon.

Facts:

Vendors are the only source of ABI-incompatibility within a major version.
The ABI for a given dependency (like, say, libuv) may match across vendors, even though the ABI for another dependency (like, say, OpenSSL) does not (I'm unsure of this last one).

Interesting use case:
An addon uses N-API and libuv. Its maintainer needs to ship binaries for each vendor libuv ABI. Once they've done that, they can go for essentially years without shipping again, because N-API doesn't change at all, and libuv changes very rarely, even across major Node.js versions.

@rvagg

@rvagg nice, that'd be great! We'll also need one for Electron v5, which is in the beta process right now. Should we open a new issue for that or can we continue that in this thread?

chore: use new NMV of 69. Refs: nodejs/TSC#651

Ping on ^. We're currently using NODE_MODULE_VERSION=68 (somewhat by accident) in Electron v5. It looks like node will be skipping past 68, so there should be no conflict there.

Before this happens again for Electron v6, we'd like to reserve a NODE_MODULE_VERSION. Should we open a new issue?

If there's not a single agreed-upon clear path forward here, it might be time to add the tsc-agenda label again.

@nornagon

@nornagon 68 is taken for the V8 7.1 upgrade, see nodejs/node#23423

I've added 69 for Electron 4 and 70 for Electron 5 in nodejs/node#24114

  { "modules": 70, "runtime": "electron", "variant": "electron",             "versions": "5" },
  { "modules": 69, "runtime": "electron", "variant": "electron",             "versions": "4" },
  { "modules": 68, "runtime": "node",     "variant": "v8_7.1",               "versions": "12.0.0-pre" },
  { "modules": 67, "runtime": "node",     "variant": "node",                 "versions": "11" },

The onus is on us to make sure we don't step on those toes. Maybe Node 12 gets 71.

@Trott I don't think there's really disagreement here, there's multiple approaches but they don't necessarily conflict, it's more about ideals and timeframes. I think we can just move forward on making nodejs/node#24114 workable and merged, then we can progress from there with alternative approaches to this whole thing.

FYI the 7.3 PR nodejs/node#25852 is getting 71. It might not need another bump for Node 12 since the ABI pieces will probably be stable after V8 7.3 is landed.

It might need another bump if we can get V8 7.4 in.

@nornagon

@nornagon newer version of Electron should definitely not have a lower NODE_MODULE_VERSION

In seriousness though, I agree though and I think we should bump the NODE_MODULE_VERSION in Electron 5 beta. Looks like we're free to take 70 so let's do that.

I think that the ABI version registry @ nodejs/node#24114 is ready to land. I've renamed it to "abi_version_registry.json" and bumped all of the module version numbers into a "NODE_MODULE_VERSION" property of the parent object, so now it's extensible and should be able to adapt to future approaches to dealing with ABI matching (if it requires a registry). For now though, it's a helpful record of who's using what and it'll be a good record for that into the future even if we move to a new mechanism.

Please review and register any objections so we can get this in and on the official record.

@codebytere

@codebytere did you get the number you needed and if so can we close this issue?

Navigation Menu

Repository navigation

NODE_MODULE_VERSION for Electron Major Releases #651

12 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

NODE_MODULE_VERSION for Electron Major Releases #651

Description

Activity

mcollina commented on Jan 9, 2019

mhdawson commented on Jan 9, 2019

mhdawson commented on Jan 9, 2019

MarshallOfSound commented on Jan 9, 2019

Trott commented on Jan 9, 2019

mhdawson commented on Jan 10, 2019

gabrielschulhof commented on Jan 14, 2019

mcollina commented on Jan 14, 2019

gabrielschulhof commented on Jan 14, 2019

gabrielschulhof commented on Jan 14, 2019

MylesBorins commented on Jan 23, 2019

12 remaining items

mhdawson commented on Jan 30, 2019

rvagg commented on Jan 30, 2019

gabrielschulhof commented on Jan 30, 2019

codebytere commented on Jan 31, 2019

nornagon commented on Mar 8, 2019

Trott commented on Mar 8, 2019

rvagg commented on Mar 12, 2019

rvagg commented on Mar 12, 2019

targos commented on Mar 12, 2019

miniak commented on Mar 13, 2019

nornagon commented on Mar 14, 2019

rvagg commented on Mar 17, 2019

mhdawson commented on Apr 3, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions