Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

refine workaround for node ABI issue with Electron install #156

Merged
merged 1 commit into from
Feb 5, 2019

Conversation

shiftkey
Copy link
Contributor

@shiftkey shiftkey commented Feb 5, 2019

This builds upon #154 now that Electron has bumped the ABI for v4 to 69 in electron/node@8bc5d17 which is available in Electron 4.0.4.

The patches applied to prebuild-install are here: prebuild/prebuild-install@099c704...8723b8b but the important part is the runtime check:

if (rc.runtime === 'electron' && rc.target[0] === "4" && rc.abi === "64") {
  log.error(`Electron version ${rc.target} found - skipping prebuild-install work due to known ABI issue`)
  log.error('More information about this issue can be found at https://github.com/lgeiger/node-abi/issues/54')
  process.exit(1)
}

This check now means prebuild-install aborts when:

  • it's the Electron runtime
  • the target is in the 4.x.x series
  • the ABI associated with it is the known incorrect one

TODO:

prebuild-install WARN install No prebuilt binaries found (target=4.0.4 runtime=electron arch=x64 libc= platform=darwin)
  • test against 4.0.3 build -> ❌
prebuild-install ERR! Electron version 4.0.3 found - skipping prebuild-install work due to known ABI issue
prebuild-install ERR! More information about this issue can be found at https://github.com/lgeiger/node-abi/issues/54

@shiftkey shiftkey changed the title refine workaround for node ABI issue with Electron install [WIP] refine workaround for node ABI issue with Electron install Feb 5, 2019
@shiftkey shiftkey changed the title [WIP] refine workaround for node ABI issue with Electron install refine workaround for node ABI issue with Electron install Feb 5, 2019
@shiftkey shiftkey requested a review from daviwil February 5, 2019 13:17
Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

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

Looks good to me! I wonder if it makes sense to PR this change upstream since the real fix only applies to 4.0.4+? Lots of people will be having this same issue.

@shiftkey
Copy link
Contributor Author

shiftkey commented Feb 5, 2019

@daviwil yeah, it's not a problem that's going to resolve itself unless people upgrade to the latest Electron 4.x versions. I can open a PR to see if upstream is interested in accepting the workaround in the interim.

@shiftkey shiftkey merged commit 717ee74 into master Feb 5, 2019
@shiftkey shiftkey deleted the refine-workaround-for-electron branch February 5, 2019 17:33
@shiftkey
Copy link
Contributor Author

shiftkey commented Feb 6, 2019

Upstreamed the workaround: prebuild/prebuild-install#95

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants