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

Feature request:child_process.exec for powershell #21905

Closed
tkamenoko opened this issue Jul 20, 2018 · 5 comments
Closed

Feature request:child_process.exec for powershell #21905

tkamenoko opened this issue Jul 20, 2018 · 5 comments
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@tkamenoko
Copy link
Contributor

  • Version:v10.7.0
  • Platform:Windows 10 64bit
  • Subsystem:child_process

On Windows, child_process.exec (or other methods) supports only cmd.exe as the shell, so running the following shows error messages.

const ChildProcess = require("child_process")

// trying to use powershell core(pwsh).
ChildProcess.exec("echo foo", { shell: "pwsh" }, (error, stdout, stderr) => {
    if (error) {
        console.error("exec error: ", error);
        return;
    }
    console.log("stdout: ", stdout);
    console.log("stderr: ", stderr);
})

result:

exec error:  { Error: Command failed: echo foo
The argument '/d' is not recognized as the name of a script file. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

    at ChildProcess.exithandler (child_process.js:291:12)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:961:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:248:5) killed: false, code: 64, signal: null, cmd: 'echo foo' }

However, there are alternate shells like powershell, git-bash, etc. So I suggest to fix the behavior.

I suppose the cause is this code(a part of child_process.normalizeSpawnArguments):

node/lib/child_process.js

Lines 469 to 479 in c3f8dd6

if (options.shell) {
const command = [file].concat(args).join(' ');
if (process.platform === 'win32') {
if (typeof options.shell === 'string')
file = options.shell;
else
file = process.env.comspec || 'cmd.exe';
args = ['/d', '/s', '/c', `"${command}"`];
options.windowsVerbatimArguments = true;
} else {

On Windows, it sets /d /s /c for args even if options.shell!=="cmd.exe". Powershell can receive -c option like unix shells, so /d /s /c should be used for only cmd.exe.

@gireeshpunathil
Copy link
Member

The current behvior is well understood and backed up by documentation:

The shell should understand the -c switch on UNIX or /d /s /c on Windows. On Windows, command line parsing should be compatible with 'cmd.exe'.

However, I see it is reasonable to accommodate powershell. cc @nodejs/child_process in case if anyone has concerns.

@tkamenoko - are you willing to contribute through a PR?

@gireeshpunathil gireeshpunathil added feature request Issues that request new features to be added to Node.js. child_process Issues and PRs related to the child_process subsystem. labels Jul 20, 2018
@richardlau
Copy link
Member

Related: #19947

@tkamenoko
Copy link
Contributor Author

@gireeshpunathil

Yes, I want to contribute!

Well, I have 2 questions before coding.
1: Where should I place the test? test/pseudo-tty?
2: Is it enough to test with cmd and powershell? Need git-bash?

Thanks.

@gireeshpunathil
Copy link
Member

awesome, thanks!

Where should I place the test? test/pseudo-tty?

test/parallel/test-chilld-process-*

Is it enough to test with cmd and powershell? Need git-bash?

would be great if you can cover git-bash too, while we are here.

@tkamenoko
Copy link
Contributor Author

Thank you for the advice. I'll start working.

tkamenoko added a commit to tkamenoko/node that referenced this issue Jul 23, 2018
On Windows, normalizeSpawnArguments set "/d /s /c" for any shells.
It cause exec and other methods are limited to cmd.exe as a shell.

Powershell and git-bash are often used instead of cmd.exe,
and they can recieve "-c" switch like unix shells.
So normalizeSpawnArguments is changed to set "/d /s /c" for cmd.exe,
and "-c" for others.

Fixes: nodejs#21905
tkamenoko added a commit to tkamenoko/node that referenced this issue Jul 26, 2018
On Windows, normalizeSpawnArguments set "/d /s /c" for any shells.
It cause exec and other methods are limited to cmd.exe as a shell.

Powershell and git-bash are often used instead of cmd.exe,
and they can recieve "-c" switch like unix shells.
So normalizeSpawnArguments is changed to set "/d /s /c" for cmd.exe,
and "-c" for others.

Fixes: nodejs#21905
joaocgreis pushed a commit to tkamenoko/node that referenced this issue Sep 1, 2018
On Windows, normalizeSpawnArguments set "/d /s /c" for any shells.
It cause exec and other methods are limited to cmd.exe as a shell.

Powershell and git-bash are often used instead of cmd.exe,
and they can recieve "-c" switch like unix shells.
So normalizeSpawnArguments is changed to set "/d /s /c" for cmd.exe,
and "-c" for others.

Fixes: nodejs#21905
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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants