Skip to content
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,Windows: deprecate explicit use of cmd.exe #14157

Closed
refack opened this issue Jul 10, 2017 · 5 comments
Closed

child_process,Windows: deprecate explicit use of cmd.exe #14157

refack opened this issue Jul 10, 2017 · 5 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@refack
Copy link
Contributor

refack commented Jul 10, 2017

  • Version: *
  • Platform: Windows
  • Subsystem: child_process

https://github.com/nodejs/node/blob/master/lib/child_process.js#L444 has a fallback to use cmd.exe in case process.env.ComSpec is falsy.
This is redundant, fragile, and generally covers-up an invalid state (%ComSpec% should always be defined and point to a valid shell executable).
This code path should be deprecated according to the guide at https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#deprecations
and https://github.com/nodejs/node/blob/master/doc/api/deprecations.md

Ref: #14149 (comment)

@refack refack added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Jul 10, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jul 10, 2017

@nodejs/platform-windows

@tniessen
Copy link
Member

Technically, this is correct, and I don't see a chance of breaking code. Some will probably break due to custom child process environments, but that's what deprecation is for.

@joaocgreis
Copy link
Member

So, for users, what this is effectively deprecating is using an environment without %COMSPEC%.

I'm -0 on this. While situations without %COMSPEC% are probably quite messed up already, cmd.exe is a valid fallback and I don't see a pressing reason for deprecating, how this actually translates as an improvement for node.

But discussion is always welcome!

@refack
Copy link
Contributor Author

refack commented Jul 11, 2017

I'm -0 on this. While situations without %COMSPEC% are probably quite messed up already, cmd.exe is a valid fallback and I don't see a pressing reason for deprecating, how this actually translates as an improvement for node.

But discussion is always welcome!

  1. I opened this as an issue rather than a PR mainly for discussion 😃
  2. Literal command strings IMHO are just bad and need a very good reason to exist.
  3. In this case it doesn't "solve", just improves the situation by ϵ%. Just as the environment could be missing %COMSPEC%, it could be missing %PATH% (or %WINDIR% so %WINDIR%\system32\cmd.exe isn't any better), and also the system could be missing cmd.exe completely. So if we're just patching over a messed up situation with a only a slightly less messy fix, why bother 🤷‍♂️
  4. In general I'm just +0.5 on this whole thing, I just don't like literal command strings in the code

@gireeshpunathil
Copy link
Member

Closing as it is inactive for around an year, feel free to re-open if there is anything outstanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants