-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
child_process.spawn ignores PATHEXT on Windows #6671
Comments
Does the |
@cjihrig I'm guessing it would work, but wouldn't native support for |
I'm not a Windows user, so forgive me if I'm wrong. Isn't |
On Windows, everything has an extension, but you can use |
It looks like |
.com, .exe, and .bat, anything else (such as .cmd) requires a shell ^--- my understanding |
My bad, .com probably works then. But if .cmd requires a shell, does that mean a bash script would behave the same way on *NIX? Given that a lot of apps install themselves as bash-based wrappers, that'd probably cause a lot of breakage. But ultimately, I guess this is a libuv issue then? |
Yes, but UNIX kernels support shebangs; on Windows, that's handled by cmd.exe. Rule of thumb: pass |
How about a |
My question would be 'why'? |
Because you don't want to spawn a shell if you don't have to. |
Performance claims should be backed up by numbers, else they're just assertions. Aside: there is no concept of forking in the Windows process model. |
const spawn = require('child_process').spawn
const shell = (process.argv[2] === 'true')
let i = 0
const spawnOne = () => new Promise((resolve, reject) => {
spawn('node', ['-v'], {shell})
.on('close', resolve)
.on('error', reject)
})
const next = () => Promise.resolve().then(() => {
return (i++ < 1000) ? spawnOne().then(next) : null
})
const started = Date.now()
next()
.then(() => {
const ended = Date.now()
const total = ended - started
console.log(total)
})
.catch((err) => {
console.error(err.stack)
})
|
I guess process spawning on Windows is as slow as they say it is. You're welcome to follow up with a pull request. I can't promise it gets merged but the numbers make a compelling case. |
I'll see what I can do. |
Seeing how #6784 was rejected, I'll close this issue. |
Originally reported as nodejs/node-v0.x-archive#2318.
TLDR: The workaround is to use cross-spawn (or cross-spawn-async).
The text was updated successfully, but these errors were encountered: