Conversation
|
@freitagbr PTAL |
| function execSync(cmd, opts, pipe) { | ||
| if (!common.execPath) { | ||
| common.error('Unable to find a path to the node binary. Please manually set common.execPath'); | ||
| } |
There was a problem hiding this comment.
What about execAsync? Maybe this should go in _exec.
There was a problem hiding this comment.
@freitagbr execAsync doesn't actually depend on it. It uses child_process.exec() directly, instead of our execSync which uses something like child_process.execSync(process.execPath + ' script.js'). You can do a simple search for process.execPath and you'll see it only shows up in execSync.
src/common.js
Outdated
| // Expose the path to the NodeJS binary to utilities | ||
| var isElectron = Boolean(process.versions.electron); | ||
| if (!isElectron) { | ||
| exports.execPath = process.execPath; |
There was a problem hiding this comment.
Maybe common.execPath should be configurable through shell.config?
There was a problem hiding this comment.
Probably the better spot for it. We'll have to be careful with the name we come up for in #534 (which is a different feature, that currently sounds very similar).
There was a problem hiding this comment.
Should we wait for #534 to implement that then?
There was a problem hiding this comment.
We'll just wait on #534. I'm not convinced if that's the best approach, since it's something that can easily be implemented by users of our module.
267dacc to
732ff33
Compare
|
Blocked on #641. I'll update this after that's merged. |
|
#641 is merged, so this can go through now. |
Switch to using common.execPath instead of process.execPath directly and warn electron users if we were unable to find the correct path to NodeJS.
732ff33 to
c303c7b
Compare
|
Updates:
📜 I didn't document the change in the README because it affects very few people. I'll document it in the wiki page for electron support after merge |
|
CI failures are unrelated, so LGTM. |
Switch to using
common.execPathinstead ofprocess.execPathdirectly and warnelectron users if we were unable to find the correct path to NodeJS.
This should be safe to merge into master because exec didn't work before anyway.
Fixes #633