-
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 parity between windows and posix #10302
Comments
Don't exec/execFile and their sync counterparts work for you? They do what you request: run the command through the command processor first. |
They don't :(
Passing {shell: true} to But the issue isn't about how I can get it to work, rather it's a difference in behavior that effects others who already are using spawn() for whatever reason, and where we're wanting to 'hook' the executable they're calling, by using a script with the same base name. On Posix, this is very easy; just make a script with the same name and chmod 755. done! On Windows, they have to change their spawn call to {shell: true}, which is unreasonable as we're trying to make it transparent. |
The other thing is CMD will also correctly search PATH, which I forgot to mention. |
Aren't you observing the documented behavior from https://nodejs.org/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows? |
Yes. And that actually applies to not just .bat or .cmd, but any extension in PATHEXT. However, I think the point of this issue is being misunderstood. There's a difference in behavior between Posix and Windows when there doesn't need to be. It would be ideal if they behaved the same when possible. Because they don't we can't support our desired use case. Is there some technical reason why, on Windows, spawn() can't just set {shell: true} when it's undefined? |
Having a file whose extension is in PATHEXT on Windows, is conceptually similar to setting execute permission on a file in Posix. Using ASSOC and FTYPE to map an extension to an executable on Windows, is conceptually similar to writing #!/some/executable in a script on Posix. I believe the kernel in Posix is reading the #! to determine how to execute the file. On Windows the same basic logic, except using PATHEXT, ASSOC, and FTYPE configurations, happens through CMD and not the kernel. I can appreciate being able to technically state spawn is explicit in it's reliance on the kernel vs a shell, but I think in practice in this case it's a distinction that doesn't help normalize the platform, which is one of the nicest things about using node. I've not found anything that breaks when launching some .exe or .com directly, vs them launching though cmd.exe, which would be the effect of spawn() defaulting shell=true on Windows. I'm sure I'm missing something? |
@papercuptech |
@gucong3000 Thank you. However, that's not quite what I need for my use case. I can't change anything in the node program that's calling I make sure our 'someprog.cmd' is higher in up in PATH, so (assuming On Posix this works as expected, on Windows it does not. @bnoordhuis @cjihrig Is this issue likely to be looked at or addressed? |
Agreed.
Because it causes spawn to not behave the same on Windows and Unix! It creates an intermediary process, so the process you think you are spawning as your child is actually your grand child. For applications that do not care if they have an intermediate process, they can set Making shell be true by default is a pretty breaking change. It would make more sense to have completely different APIs (exec vs. spawn already exists, however, wherein exec uses shell by default, and spawn does not). This should be fixed in libuv, @joshgav and @digitalinfinity, by libuv doing the registry lookup, and directly spawning the correct interpreter so there is no intermediate child process. |
Thanks @sam-github - I think @ianwjhalliday was planning on taking a crack at this once he was back from vacation. |
This is the ideal for sure.
Yeaahh.. Understood actual process would be grand child, but purely from node's API, and this is just out of curiosity, what would a caller actually see as a change in behavior?
Hmmm... Not a fan of yet another api here; three should be plenty? If fixed in uv, then no need, correct? |
|
Doh! of course. Updating uv seems the best place, except it would have to mimic something Windows is doing. Probably not a big deal in this case, but if Windows implementation changed in this area, uv would have to follow, and it would seem implementation would have to search PATH, look for any files with PATHEXT extensions (assuming command arg to spawn was extension-less), lookup ext-to-filetype in registry (which I think might not be as straightforward as one would think) and then filetype-to-realexe. All do-able for sure but maybe a minor hassle. Just wondering: Only on Windows, and only when shell === undefined, to then launch through cmd, and if there's a way to detect the 'real' process is the grand child, to return it's PID, and whatever else of it, so caller has no idea, and CMD (Windows) can still deal with resolving all the above? Might still need be done in uv, unless uv exposes enough to node to figure this out for itself? Just an idea. Regardless, it would be nice if spawn() worked the same in this case, and it does seem something in, or with help of, uv could make that happen without having to add a forth child_process method :). Thanks |
On Posix:
test file:
On Windows:
test.cmd file:
Windows uses PATHEXT, ASSOC, and FTYPE to achieve something similar to Posix using execute permission and shebangs.
On Windows,
spawn()
is not giving Windows a chance to query PATHEXT, ASSOC, FTYPE information to runtest.cmd
as an executable.To fix, on Windows,
spawn()
should always launch usingCMD
, when{shell:undefined}
(i.e. set it to true when undefined), so that CMD can correctly handle PATHEXT, ASSOC, and FTYPE configuration. This does meanreal
, binary executables won't be launched directly but this should have no perceivable impact on launch time or behavior. The benefit is increased parity between Posix and Windows behavior.The text was updated successfully, but these errors were encountered: