Detect the correct runtime when find() is called from Electron #187
Conversation
|
There's actually an existing PR #177 to address this issue, unfortunately I only noticed it after implementing my own fix. Compared to the previous PR mine is a single commit and more conformant to the existing code style. Anyway, I'm not fussed about which one gets merged in as long as the issue is fixed. |
|
Can someone merge this? I need this for my latest project... |
https://github.com/maxkorp/node-pre-gyp-npmrc-helper Ideally, I'd like to get the npm config lookup and 187 added to just node-pre-gyp. |
This fixes the case where require('node-pre-gyp').find() is called
from Electron without explicitly specifying the runtime in the options.
Previously the runtime would just default to "node" and find() would
fail to find any native modules specifically built for Electron because
`node-gyp` named the build output directory
`{runtime}-v{version}-{platform}-{arch}`. For example after building
a native module for Electron v0.34.2 the module binary ended up in the
`electron-v0.34-win32-x64` directory, but find() was looking for it in
the `node-v46-win32-x64` directory.
enlight
force-pushed
the
detect-electron-runtime-in-find
branch
from
e4fde46 to
ef79b3f
Compare
|
I can confirm this branch works. Please merge! |
|
@springmeyer any chance you can take a look ? |
|
To anyone else who stumbles in here I've created electron-node-pre-gyp-fix to monkey-patch |
The workaround consists of copying the built native modules to the directory where `node-pre-gyp` looks for them at runtime. This seems to be the only way to get `node-inspector` working properly because my attempts to monkey-patch `node-pre-gyp` seem to fail in that case (even thought monkey-patching worked for other native modules like `sqlite3`). This workaround can be removed if mapbox/node-pre-gyp#187 is ever merged in.
|
Actually, the |
|
For what its worth, with the current version, just using an npmrc with the appropriate target and versions set will suffice for npm install to download the appropriate binaries, or compile appropriately when needed. |
|
please merge this |
This patch looks good and mergable but I held back due to the above comment. @enlight - can you share more details on whether you think this should land or not? |
|
@springmeyer that comment is specific to electron-rebuild and what they are doing - i dont think it is a criticism of this patch. Currently electrons primary instructions for how to debug include installing a patched version of node-pre-gyp - http://electron.atom.io/docs/v0.37.3/tutorial/debugging-main-process/ |
|
@lukeapage is correct @springmeyer my earlier comment was in relation to one of the workarounds people have had to employ while waiting for this PR to get merged, once this PR is merged those workarounds will no longer be necessary. |
|
Thanks @enlight - I'll merge and tag |
|
Yuppi aye, aye, yuppi yuppi aye... Thanks a lot to all! |
|
This did not work for apm, but looking at this commit I tested just setting --runtime=electron and all my troubles was gone. Made a PR for apm: atom/apm#536 |
|
module.js:442 Uncaught Error: Cannot find module ...seems to be related and not fixed.
|
|
Hmm... have I hit the same (or similar) problem? Noob on Electron trying to find my way around, not sure where node-pre-gyp fits in, hitting problems with sqlite3... Using Electron 'two package.json', Electron v1.4.1, node-pre-gyp v0.6.28 from sqlite3 v3.1.4, node v6.6.0, npm v3.10.3 on Linux. Any hints what do to? |
This fixes the case where require('node-pre-gyp').find() is called from Electron without explicitly specifying the runtime in the options. Previously the runtime would just default to "node" and find() would fail to find any native modules specifically built for Electron because
node-gypnamed the build output directory{runtime}-v{version}-{platform}-{arch}. For example after building a native module for Electron v0.34.2 the module binary ended up in theelectron-v0.34-win32-x64directory, but find() was looking for it in thenode-v46-win32-x64directory.