electron / node-abi

:turtle: :rocket: Get the Node.js and Electron ABI for a given target and runtime
https://www.npmjs.com/node-abi
MIT License
164 stars 58 forks source link

Electron 11.0.0-beta.11 produces invalid builds #103

Closed Julusian closed 3 years ago

Julusian commented 3 years ago

I have been digging into an issue with a nan and prebuild based library that was throwing an error only when run in electron 11 https://github.com/serialport/node-serialport/issues/2191

My conclusion is that electron v11.0.0-beta.11...v11.0.0-beta.12 is a breaking abi change and should have changed the abi version number. I haven't raised anything upstream, as I don't see anything they can do other than say to not use that version.

So to make everything happy, prebuild (and others?) need to build for electron 11 against v11.0.0-beta.12 or newer. As they get the version number from this library, it would be nice to fix it here, but I don't know how the data should be modelled, as a single version essentially needs to be ignored.

dunkmann00 commented 3 years ago

There is a similar problem connected to electron 9 prebuilds, also coincidentally relating to serialport. In this case the electron 9 betas up to beta 9 are producing the problem, but from beta 10 and up it seems to work just fine.

@MarshallOfSound Looking at how the update-abi-registry.js script handles electron releases, would you be open to reversing the list when processing? That way instead of an ABI version linking to the earliest target (which in the case of electron builds is always an early beta), it would link to the latest released version that uses that particular ABI.

Since the update script is run automatically that wouldn't create any extra work for anyone, and I think it would solve both of these problems with prebuild.

Julusian commented 3 years ago

@dunkmann00 that will break various methods exposed by this library, such as https://github.com/lgeiger/node-abi/blob/master/index.js#L16.

I believe the primary purpose of this data is to figure out what abi version to use for a process, and not for prebuild to choose a version to build against.

dunkmann00 commented 3 years ago

@Julusian I'm not sure why it would break something?

What I was suggesting was:

update-abi-registry.js line 42

function electronReleasesToTargets (releases) {
  const versions = releases.map(({ version }) => version)
  const versionsByModules = releases
    .filter(release => release.deps && Number(release.deps.modules) >= 70)
++  .reverse()
    .map(({ version, deps: { modules } }) => ({
      version,
      modules,
    }))
    .reduce(
      (acc, { modules, version }) => ({
        ...acc,
        [modules]: version,
      }),
      {}
    )
...
}

Just adding the reverse array method call on the line marked wiht the ++. I could be missing something but I don't think that would break anything?

Julusian commented 3 years ago

That results in the abi_registry.json containing this:

{
    "abi": "80",
    "future": false,
    "lts": false,
    "runtime": "electron",
    "target": "9.4.4"
  },
  {
    "abi": "82",
    "future": false,
    "lts": false,
    "runtime": "electron",
    "target": "12.0.0-nightly.20200914"
  },
  {
    "abi": "85",
    "future": false,
    "lts": false,
    "runtime": "electron",
    "target": "11.4.8"
  },
  {
    "abi": "87",
    "future": false,
    "lts": false,
    "runtime": "electron",
    "target": "13.0.0-nightly.20201123"
  },

Which means that a call to getAbi('11.0.0', 'electron') gives back 80, instead of the expected 85

dunkmann00 commented 3 years ago

@Julusian 👍 I see now.

MarshallOfSound commented 3 years ago

Electron 11 should produce valid builds nowadays, the ABI registry lookup logic has been tweaked slightly. Passing the correct electron version into node-abi results in the updated ABI version being used even if the ABi version during an Electron major line