Allow electron process forks of modules that use pre-gyp

[bcomnes]

closes #278

I still need to check to see if this works in built electron apps, but this is fairly close to solving the issue of allowing childProcess.forks in electron of node-pre-gyp modules.

[springmeyer]

Thanks for this PR @bcomnes and especially for the detailed description of the problem over at #278. I'm the maintainer of node-pre-gyp and happy to accept this PR once there are tests. I'm not an electron user so I'd love help on how to best set up tests to confirm this is working and ensure it never regresses (which will run on travisci). Are writing unit tests something you could do? Or could you advise on how to test manually and maybe others could help convert those into unit tests?

[bcomnes]

👌 I'll put something together today.

[bcomnes]

A decent test would be to basically try to load a node-pre-gyp module in a module running in a childProcess.fork of electron and see if it throws or not. Would you be opposed to pulling in https://github.com/electron-userland/electron-prebuilt and https://github.com/electron/electron-rebuild as a dev dependency?

Basically we would need to unit-testify https://github.com/ballpit/electron-sqlite3-fork-bug/blob/master/main.js#L14-L18

[bcomnes]

The other weird thing about this is requiring electron. Ideally we could pull off the correct electron version from the thing that actually called the fork, rather than an action-at-a-distancey require resolve of a package.json. I'll ask upstream how we can do this.

[bcomnes]

Oh crap, I just pushed some unintentional files up. Fixing.

[bcomnes]

Ok I made a few changes:

• Simplified the electron case of get_process_runtime
• No longer mutate process.versions.
• The electron version check happens in get_runtime_abi instead
• I opened an issue upstream to electron to expose the version of electron to the forked electron process somehow Make electron version available in childProcess.fork electron/electron#9058 so we can avoid the require('electron/package.json) try/catch.
• Added a mock test to test out the detection of ELECTRON_RUN_AS_NODE

Is that enough testing for this you think? Or do we want to add more of an acceptance / integration test closer to what https://github.com/ballpit/electron-sqlite3-fork-bug/blob/master/main.js tests for?

[bcomnes]

@springmeyer any feedback on the tests/current state of the PR?

EDIT: Crap, broken tests. Fixing now.

[bcomnes]

Rebased.

[bcomnes]

@springmeyer Do you have any feedback on the tests I added?

[springmeyer]

@bcomnes - made a request for a few minor changes. If you can address those then I am willing to merge. I admit that I don't fully understand the changes or how they work. So I would love if anyone else that uses electron could weight in.

[bcomnes]

Ok, cleaned up those two issue, and rebased.

To TLDR explain the issue: when forking a process in electron, a special ENV var is introduced to the forked process environment (ELECTRON_RUN_AS_NODE) and any prebuilt native dependencies need to pick this up in order to find the correct bindings, otherwise the normal checks that node-pre-gyp performs to check if its running electron fail and it returns the vanilla node bindings (or at least this is one approach).

The only real weirdness here is the electron version resolution. Ideally this would also come in from the ENV of the forked process, but its not available yet. I'm currently working with the electron team to get that in (see electron/electron#9058) and will PR that improved method once its available.

[bcomnes]

Hrmmm tests are failing now. Any idea if its because of what I'm doing?

[springmeyer]

Any idea if its because of what I'm doing?

@bcomnes - I think they may be unrelated. Can you rebase against the master branch and see if things are fixed?

[bcomnes]

Rebased. Fingers crossed!

[bcomnes]

In a spat of inspiration to make the world slightly better this morning, I squashed and rebased this.

An alternative PR for the same problem is here: #343

It looks like the difference between the two is ^^ that one requires you to pass in an ENV var yourself, this PR attempts to resolve the correct electron version for you. Personally, I think this is how it should work, but the code is a little more ugly until we can support from upstream electron.

Getting rock solid sqlite support in electron would be amazing, and this would help make that happen.

[bcomnes]

All tests passing locally. CI seems to be failing on unrelated tests 🤔 any ideas?

[bcomnes]

@springmeyer any clue whats going on in the failed runs? Also what happened to travis?

[dimalo]

Hey guys, thanks for your work! Please make this merge finally happen!

[springmeyer]

Thanks so much @bcomnes. Would love to land this. At the same time I don't have time or ability to review or merge currently. Nor debug travis, but it might be something like mapbox/gzip-hpp#33 which resulted from .org -> .com issues I think.

[JakeRadMSFT]

@bcomnes it looks like electron made the change you wanted:
electron/electron#9058

[bcomnes]

@JakeRadMSFT The thing that I wanted to remove was https://github.com/mapbox/node-pre-gyp/pull/279/files#diff-314ae8d53157a25e4458ced25e711105R87

electron/electron#16450 adds a process.versions.electron which suposedly provides that. Let me try it out.

[bcomnes]

Ok, re-reading this. Totally untested, but: IF process.versions.electron is added into forks, as noted in electron/electron#9058, then this PR is no longer needed. Since tests arn't passing on master, I dont even think the process.env.ELECTRON_RUN_AS_NODE part is needed.

Closing.

[awohletz]

Confirmed that updating to Electron v6 (currently in beta) fixed the issue for me. E.g. I am able to use node-sqlite3 from an Electron-forked process.