fix: ABI registry updater

[vecerek]

Object mutation is evil 👿

This PR fixes the bug in the ABI registry updater that caused incorrect ABI values to be mapped to different Node runtime versions.

Fixes #90 and #93. Also updates the tests. Makes #94 obsolete.

CC: @MarshallOfSound

[ludiazv]

Please merge this PR. This breaks prebuilded packages with prebuildify. In fact as Node 12 is curent LTS version.

[vecerek]

@ludiazv I was having issues with prebuildify and that's how I got to investigating node-abi and opening this PR in the first place 😄

[malept]

Also, the PR title should be in Conventional Commits format since the PR is likely going to get squashed.

[vecerek]

Thanks, @malept, for the review. I committed your suggestion and changed the PR title to adhere to CC standards. Let me know if there's anything else needed to be changed.

[malept]

I made an additional comment that I think got lost in my suggestion:

I think that the Electron code needs to be updated so that the generated targets are always x.0.0-beta.1 but the only thing that changes when there's a stable release is the future property.

Do you mind doing this as well? I think otherwise the tests will continue to fail (I have no idea why the tests aren't running for PRs...).

[vecerek]

@malept I'm trying to understand what the real issue is here with generating the ABI versions for electron. And what exactly are you asking me to change 🙂

The ABI updater fetches the abi version from here and the electron version from here. I discovered a discrepancy between these two "sources of truth".

What the ABI source says about electron v11 is that it uses modules v85.

{
  "NODE_MODULE_VERSION": [
    { "modules": 85, "runtime": "electron", "variant": "electron",             "versions": "11" },
    { "modules": 84, "runtime": "node",     "variant": "v8_8.3",               "versions": "15.0.0-pre" },
    { "modules": 83, "runtime": "node",     "variant": "v8_8.1",               "versions": "14.0.0" },
    { "modules": 82, "runtime": "electron", "variant": "electron",             "versions": "10" }
  ]
}

What the electron source says about Electron v11.0.0-beta.1 is that it uses modules 82 (it does not list an electron version 11.0.0):

{
  "node_id": "MDc6UmVsZWFzZTMwMTY4NjQ4",
  "tag_name": "v11.0.0-beta.1",
  "name": "electron v11.0.0-beta.1",
  "prerelease": true,
  "published_at": "2020-08-26T22:41:19Z",
  "version": "11.0.0-beta.1",
  "npm_package_name": "electron",
  "deps": {
    "node": "12.18.3",
    "v8": "8.6.373-electron.0",
    "uv": "1.38.0",
    "zlib": "1.2.11",
    "openssl": "1.1.0",
    "modules": "82",
    "chrome": "86.0.4234.0"
  },
  "npm_dist_tags": [],
  "total_downloads": 401
}

Furthermore, how the ABI updater was designed, I assume, was that if it doesn't find an Electron version satisfying a major stable version (like v11 coming from the ABI source), then it appends -beta.1 to the target. This is where the discrepancy comes in because the module assigned to v11.0.0-beta.1 will be 85 (from ABI source) instead of 82 (from the Electron source). So, which of the 2 sources is correct?

[malept]

And what exactly are you asking me to change :slightly_smiling_face:

Ideally, what would happen is you'd take the Electron version from the Node registry and append .0.0-beta.1 to it, regardless of whether it's stable or not. That should cause the tests to pass.

So, which of the 2 sources is correct?

Well, it's complicated. The Node registry is supposed to be the source of truth but humans are fallible. I reported the bug and the Releases WG very quickly put together some fixes. I think the correct way to do this is to use the Node registry, and mismatches should be reported to the Electron Releases WG.

[kriszyp]

Is there anything that can be done to help? I think there are still lot of builds that are broken, waiting for this fix.

[malept]

Is there anything that can be done to help? I think there are still lot of builds that are broken, waiting for this fix.

At this point I'm just waiting on a response to my latest comment.

[vecerek]

@malept Sorry for the late reply. I'm still trying to wrap my head around this. I tried your suggestion and updated the Electron code, so that the generated targets are always x.0.0-beta.1. This has fixed one test but broken two other tests in return:

t.equal(getTarget('82', 'electron'), '10.0.0')

/*
operator: equal
    expected: |-
      '10.0.0'
    actual: |-
      '10.0.0-beta.1'
*/

t.equal(getTarget('76', 'electron'), '8.0.0')

/*
operator: equal
    expected: |-
      '8.0.0'
    actual: |-
      '8.0.0-beta.1'
*/

The reason why getAbi('10.0.0-beta.1', 'electron') == 80 instead of 82 is in the implementation of getAbi:

for (var i = 0; i < allTargets.length; i++) {
  var t = allTargets[i]
  if (t.runtime !== runtime) continue
  if (semver.lte(t.target, target)) abi = t.abi
  else break
}

This loops over "all" the targets:

var allTargets = deprecatedTargets
  .concat(supportedTargets)
  .concat(additionalTargets)
  .concat(futureTargets)

and sets the ABI version to the target's version unless the target is larger based on semver comparison. When it comes to 10.0.0-beta.1, it is larger than 9.0.0 but smaller than 10.0.0. Hence, the ABI version is stuck at the version of 9.0.0. Also, it is not always the case that the ABI version of beta versions equals to the ABI version of the stable ones (like in the case of 10.0.0-beta.1). For example, 9.0.0 has an ABI of 80, and 9.0.0-beta.1 has an ABI of 76.

Currently, allTargets does not include any beta version electron targets except for 11.0.0-beta.1. Since there's no strict rule to what ABI version is included in beta/stable versions, I don't see how the correct ABI version could be returned for beta versions of Electron without including them in allTargets.

I suggest fixing Electron ABI versions in a separate PR. In that case, I would have to drop https://github.com/lgeiger/node-abi/pull/95/commits/1050f3d79f5049362ff65568216d7fa639643db0 first.

[vecerek]

@malept I gave it another shot, decided to take a different route, and refactored the processing of electron targets a little bit. With this implementation, all tests are passing now.

[vecerek]

Hi @cb1kenobi and thanks for noticing the issue. It was caused by an inconsistency in the abi version registry:

[
  { "modules": 88, "runtime": "node",     "variant": "v8_8.6",               "versions": "15.0.0" },
  { "modules": 83, "runtime": "node",     "variant": "v8_8.1",               "versions": "14.0.0" },
  { "modules": 79, "runtime": "node",     "variant": "v8_7.8",               "versions": "13" },
  { "modules": 72, "runtime": "node",     "variant": "node",                 "versions": "12" },
  { "modules": 67, "runtime": "node",     "variant": "node",                 "versions": "11" },
]

Fixed it by properly coercing the versions and updated the tests to include v14 and v15 as well. This uncovered an issue with sorting in loadGeneratedTargets for Node versions <= 10 (not an issue in v14, didn't test in v12), so I fixed it in https://github.com/lgeiger/node-abi/pull/95/commits/dc36ed463c9277b2fa08e65c2f4bb9fd215215f8. I also decided to refactor the derivation of node targets similarly to my electron refactoring in https://github.com/lgeiger/node-abi/pull/95/commits/250d7d7b38da4f01cfa75f2ed7a9b693aa6830d3.

[vecerek]

@malept I believe I addressed your comments, may I ask you to re-review the PR? 🙂

[vecerek]

@malept bump 🙂 cc @lgeiger

[vecerek]

Another week went by, another bump 🙂 Apparently, there's quite some number of tools depending on this package. It would be nice to get this in asap. cc @malept @lgeiger @MarshallOfSound @mafintosh