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

pre-gyp looks for the wrong ABI in electron childProcess.forks #278

Open
bcomnes opened this Issue Mar 28, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@bcomnes

bcomnes commented Mar 28, 2017

It appears node-pre-gyp modules are trying to use the wrong ABI when run inside of a childProcess.fork from an electron process, even if you have rebuilt the correct ABI using something like electron-rebuild. It looks like this problem has been around a while, and even a few hacks came and went.

Here is an example of the issue with sqlite3: https://github.com/ballpit/electron-sqlite3-fork-bug/blob/master/main.js

It looks like node-gyp-build had a similar issue a while back, and they ended up implementing a check for the process.env.ELECTRON_RUN_AS_NODE and returning electron bindings when thats found, and it looks like maybe we need to port that over to node-pre-gyp as well to fix this problem.

It looks like the ABI selection is happening in https://github.com/mapbox/node-pre-gyp/blob/master/lib/util/versioning.js. Any idea where such a check might live in that file?

@bcomnes bcomnes changed the title from pre-gyp looks for the wrong ABI in electron childProcess.fork's to pre-gyp looks for the wrong ABI in electron childProcess.forks Mar 28, 2017

bcomnes added a commit to bcomnes/node-pre-gyp that referenced this issue Mar 29, 2017

bcomnes added a commit to bcomnes/node-pre-gyp that referenced this issue Apr 12, 2017

bcomnes added a commit to bcomnes/node-pre-gyp that referenced this issue May 4, 2017

bcomnes added a commit to bcomnes/node-pre-gyp that referenced this issue May 5, 2017

@Tomyail

This comment has been minimized.

Show comment
Hide comment
@Tomyail

Tomyail Jan 12, 2018

any update for this issue? I need this pull request

Tomyail commented Jan 12, 2018

any update for this issue? I need this pull request

@Tomyail

This comment has been minimized.

Show comment
Hide comment
@Tomyail

Tomyail Jan 12, 2018

I create a pull request for this issue

Tomyail commented Jan 12, 2018

I create a pull request for this issue

@bcomnes

This comment has been minimized.

Show comment
Hide comment
@bcomnes

bcomnes Sep 11, 2018

This is my original PR to close this: #279

Open to either solution, but I think we should aim to make this work without user provided env vars.

bcomnes commented Sep 11, 2018

This is my original PR to close this: #279

Open to either solution, but I think we should aim to make this work without user provided env vars.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment