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

Fix Electron 3 ABI #55

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@bendemboski
Copy link

commented Jan 9, 2019

Electron 3's ABI was incorrectly specified as 64 instead of 59, and now that Electron 4 has shipped and actually has ABI version 64, this is causing prebuild/prebuild-install to try to share binaries between Electron 3 and 4 which doesn't work.

Fixes #54

Fix Electron 3 ABI
Electron 3's ABI was incorrectly specified as 64 instead of 59, and now that Electron 4 has shipped and actually has ABI version 64, this is causing prebuild/prebuild-install to try to share binaries between Electron 3 and 4 which doesn't work.

Fixes #54
@MarshallOfSound

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

The ABI of Electron 3 is 64, you can verify this with electron --abi with Electron 3.x installed. The thing is, although the node team are very careful with ABI compatibiility, between Electron 3 and Electron 4 we switched from GYP to GN and OpenSSL to BoringSSL (amongst other things) in such a way that the builds probably aren't 100% compatible.

Their are solutions to this being worked on upstream so that Electron can reserve ABI values independently of Node (along with other channels of distribution).

nodejs/TSC#621

Not sure what the best solution is for this right now but this isn't it.

@bendemboski

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

Hmm...does this tag not actually correspond to the 3.0.0 release then? I see that Electron 3.0.0 reports Node 10.2.0, even though the 3.0.0 tag lists a 9.7 version in DEPS.

Regardless, I'm pretty sure that the actual ABI differs between Electron 3 and Electron 4. Here is a keytar.node binary built out of the master of node-keytar by running ./node_modules/.bin/prebuild -t 3.0.0 -r electron:

keytar.node.zip

Electron 3 can load it, but Electron 4 can't, and fails with

dyld: lazy symbol binding failed: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /private/tmp/elec4/node_modules/keytar/build/Release/keytar.node
  Expected in: flat namespace

dyld: Symbol not found: __ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorE
  Referenced from: /private/tmp/elec4/node_modules/keytar/build/Release/keytar.node
  Expected in: flat namespace

A symbol dump includes

U v8::FunctionTemplate::New(v8::Isolate*, void (*)(v8::FunctionCallbackInfo<v8::Value> const&), v8::Local<v8::Value>, v8::Local<v8::Signature>, int, v8::ConstructorBehavior)

Since Electron 3 can load it, its v8 must not include these changes to the FunctionTemplate::New signature (among others), which was first introduced in v8 6.7, but was patched into Node 10.0, even though it (Electron 3) reports Node 10.2 as its Node version.

Node 10.2 (running independent of Electron), on the other hand, cannot load this binary, and fails with the same error as Electron 4.

So I'm not entirely sure what's going on here, but I'm pretty sure Electron 3 and 4 do not have ABI-compatible versions of v8::FunctionTemplate::New, and I'm also pretty sure that Electron 3's v8 is binary-incompatible with Node 10.2's even though Electron 3 includes Node 10.2.

The end result is that binaries built for Electron 3 that reference any of those symbols that had a SideEffectType argument added fail to load in Electron 4 even though the two report the same ABI.

So I'm seeing that this isn't really a bug in node-abi because it's just report what Electron reports ABI-wise -- perhaps this is a bug in Electron itself?

@bendemboski

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

The result being that I think all packages using prebuild cannot be consumed in Electron 4 projects without forcing a local rebuild -- see here.

@MarshallOfSound

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

@bendemboski We changed build systems from 3.0 to 4.0

The node used in the GYP build is in the vendor/node directory. You can see this for 3.0.0 here and can see that it is 10.2

https://github.com/electron/node/tree/3a619a78bcbf105f6c7d48a2eeeda919111fbc0f

As mentioned above there may be an incompatibility and there is active work upstream to determine how best to handle downstream distributions of node and their ABI compatibility. This issue is not a bug in Electron or node-abi rather a fundamental issue with how ABI numbers are currently chosen/declared. Once a solution is chosen for that we will be able to declare our own ABI numbers.

@MarshallOfSound
Copy link
Collaborator

left a comment

Requesting changes to prevent merge

@bendemboski

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

@MarshallOfSound thanks for taking the time to explain this to me -- I'm new to chasing around Electron's Node/v8 dependencies and ABI versions, and it seems to be a kinda steep learning curve 😄

The core issue that prebuilt packages such as keytar cannot run against Electron 4 because Electron 3 and 4 have different binary interfaces is starting to cause widespread problems as people upgrade to Electron 4 (see here here here, maybe here) and I think preventing Electron 4 adoption.

Can you point me to where the discussions are happening so I can track the status of a fix to these issues, or suggest where else I might raise this issue?

@MarshallOfSound

This comment has been minimized.

Copy link
Collaborator

commented Jan 9, 2019

@bendemboski We've raised an issue with the Node.JS TSC to get some unique ABI numbers for Electron so that we can assign a new Electron specific ABI to Electron 4. We'll update this PR / issue with information as we figure it out.

🔗 nodejs/TSC#651

@bendemboski

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

Great, thanks, I'll follow that issue for updates.

@MarshallOfSound

This comment has been minimized.

Copy link
Collaborator

commented Feb 3, 2019

Electron 4.0.4 now has a dedicated ABI number of 69.

Refs: #57
Refs: electron/node@8bc5d17

@catdad catdad referenced this pull request Feb 12, 2019

Closed

update to electron 4 #89

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.