win: fix spawning npm #635
Conversation
Fixes spawning NPM under Windows. Fixes: nodejs#634
Codecov Report
@@ Coverage Diff @@
## master #635 +/- ##
==========================================
- Coverage 95.46% 95.24% -0.22%
==========================================
Files 26 26
Lines 816 821 +5
==========================================
+ Hits 779 782 +3
- Misses 37 39 +2
Continue to review full report at Codecov.
|
|
/cc @nodejs/citgm |
| if (context.options.testPath) { | ||
| options.env.PATH = `${context.options.testPath}:${process.env.PATH}`; | ||
| nodeBin = which('node', { | ||
| path: options.env.PATH || process.env.PATH |
There was a problem hiding this comment.
Can options.env.PATH ever not be process.env.PATH outside of this if? (i.e. does the nodeBin calculation need to occur outside of the if statement?)
|
The code being very similar to what we have in https://github.com/nodejs/citgm/blob/b3b60c38152925661362e2f725ca8e7e8e52ab0a/lib/package-manager/test.js, would it make sense to extract it to a common function? |
| options.env.PATH = `${binDirectory}:${process.env.PATH}`; | ||
|
|
||
| const proc = spawn(packageManagerBin, args, options); | ||
| const proc = spawn(nodeBin, args, options); |
There was a problem hiding this comment.
does this still work if the package manager is a script? Say I halve aliased npm to some
#!/usr/bin/env bash
node /path/to/npm.js --log-level=silly $0More real script - the recommended way of installing yarn on mac is through brew - and they use a script not a JS file. So node cannot execute the file. See #560 (comment)
|
There is an alternative fix already proposed here: 84b5048 |
Fixes spawning npm under Windows. CITGM would try to spawn
npm-cli.jsdirectly, causing Windows Script Host to get used to run the npm code.Fixes: #634
Checklist
npm testpasses (linter passes, tests are broken on Windows)