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.spawn adds flags that might cause non-native shells to fail on windows #19947

Closed
idanpa opened this issue Apr 11, 2018 · 5 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. wsl Issues and PRs related to the Windows Subsystem for Linux.

Comments

@idanpa
Copy link

idanpa commented Apr 11, 2018

  • Version: 8.11.1
  • Platform: Windows 10 64 bit
  • Subsystem: child_process

On Windows, when child_process.spawn is given the optional shell parameter (#4598) it makes an assumption that the shell would understand the /c /s flags (cmd.exe or powershell).
This limits the option to use bash (using WSL) on windows.
Can by bypassed by:

args = ["\"", cmd].concat(args, "\"");
currentSync = child.spawn("bash -c", args, {stdio: 'pipe', shell: "cmd.exe"});

But would be nice to have a better solution.

Thanks,
Idan

@vsemozhetbyt
Copy link
Contributor

FWIW, we warn about this in docs:

https://nodejs.org/api/child_process.html#child_process_shell_requirements

@vsemozhetbyt vsemozhetbyt added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. wsl Issues and PRs related to the Windows Subsystem for Linux. labels Apr 11, 2018
@bnoordhuis
Copy link
Member

Yep, and it doesn't seem like an unreasonable restriction to me. I don't really see how we could improve it either.

@idanpa
Copy link
Author

idanpa commented Apr 11, 2018

@bnoordhuis what do you say about these options:

  1. Having shellOptions arg with default value '/d /s /c' on windows and '-sc' on unix
  2. Skip adding the options if shell string already contains flags (having either " -" or " /"
  3. Having 2 different optional args - shshell and ntshell that gets the right options accordingly (without checking the platform)

@bnoordhuis
Copy link
Member

(1) and (3) are fixing a problem that isn't really a problem (don't use .shell, spawn the shell directly) and (2) is the kind of heuristic that can have false positives (e.g. / is also a path separator.)

I mean, yes, there are ways to solve your issue with more API options but the extra complexity doesn't seem worth it.

@bnoordhuis
Copy link
Member

I'll close this out. It's been two weeks and no one spoke out in favor of making changes.

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. wsl Issues and PRs related to the Windows Subsystem for Linux.
Projects
None yet
Development

No branches or pull requests

3 participants