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: support any shells on Windows #21943

Closed
wants to merge 16 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions doc/api/child_process.md
Expand Up @@ -415,7 +415,7 @@ changes:
[Default Windows Shell][]. **Default:** `false` (no shell).
* `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
done on Windows. Ignored on Unix. This is set to `true` automatically
when `shell` is specified. **Default:** `false`.
when `shell` is specified and is CMD. **Default:** `false`.
* `windowsHide` {boolean} Hide the subprocess console window that would
normally be created on Windows systems. **Default:** `true`.
* Returns: {ChildProcess}
Expand Down Expand Up @@ -867,7 +867,7 @@ changes:
[Default Windows Shell][]. **Default:** `false` (no shell).
* `windowsVerbatimArguments` {boolean} No quoting or escaping of arguments is
done on Windows. Ignored on Unix. This is set to `true` automatically
when `shell` is specified. **Default:** `false`.
when `shell` is specified and is CMD. **Default:** `false`.
* `windowsHide` {boolean} Hide the subprocess console window that would
normally be created on Windows systems. **Default:** `true`.
* Returns: {Object}
Expand Down Expand Up @@ -1432,8 +1432,9 @@ to `stdout` although there are only 4 characters.

## Shell Requirements

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'`.
The shell should understand the `-c` switch. If the shell is `'cmd.exe'`, it
should understand the `/d /s /c` switches and command line parsing should be
compatible.

## Default Windows Shell

Expand Down
11 changes: 8 additions & 3 deletions lib/child_process.js
Expand Up @@ -469,14 +469,19 @@ function normalizeSpawnArguments(file, args, options) {

if (options.shell) {
const command = [file].concat(args).join(' ');

// Set the shell, switches, and commands.
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;
// '/d /s /c' is used only for cmd.exe.
if (/^(?:.*\\)?cmd(?:\.exe)?$/i.test(file)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A late comment, I think that \b might be a simpler test for the prefix:

if (/\bcmd(\.exe)?$/i.test(file)) 

also IMHO no need to ?: the groups since we just .test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. I would call my comment a "nit" (that is a non blocking style change)

args = ['/d', '/s', '/c', `"${command}"`];
options.windowsVerbatimArguments = true;
} else {
args = ['-c', command];
}
} else {
if (typeof options.shell === 'string')
file = options.shell;
Expand Down
68 changes: 68 additions & 0 deletions test/parallel/test-child-process-exec-any-shells-windows.js
@@ -0,0 +1,68 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const fs = require('fs');
const tmpdir = require('../common/tmpdir');

// This test is only relevant on Windows.
if (!common.isWindows)
common.skip('Windows specific test.');

// This test ensures that child_process.exec can work with any shells.

tmpdir.refresh();
const tmpPath = `${tmpdir.path}\\path with spaces`;
fs.mkdirSync(tmpPath);

const test = (shell) => {
cp.exec('echo foo bar', { shell: shell },
common.mustCall((error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes('foo') && stdout.includes('bar'));
}));
};
const testCopy = (shellName, shellPath) => {
// Copy the executable to a path with spaces, to ensure there are no issues
// related to quoting of argv0
const copyPath = `${tmpPath}\\${shellName}`;
fs.copyFileSync(shellPath, copyPath);
test(copyPath);
};

const system32 = `${process.env.SystemRoot}\\System32`;

// Test CMD
test(true);
test('cmd');
testCopy('cmd.exe', `${system32}\\cmd.exe`);
test('cmd.exe');
test('CMD');

// Test PowerShell
test('powershell');
testCopy('powershell.exe',
`${system32}\\WindowsPowerShell\\v1.0\\powershell.exe`);
fs.writeFile(`${tmpPath}\\test file`, 'Test', common.mustCall((err) => {
assert.ifError(err);
cp.exec(`Get-ChildItem "${tmpPath}" | Select-Object -Property Name`,
{ shell: 'PowerShell' },
common.mustCall((error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes(
'test file'));
}));
}));

// Test Bash (from WSL and Git), if available
cp.exec('where bash', common.mustCall((error, stdout) => {
if (error) {
return;
}
const lines = stdout.trim().split(/[\r\n]+/g);
for (let i = 0; i < lines.length; ++i) {
const bashPath = lines[i].trim();
test(bashPath);
testCopy(`bash_${i}.exe`, bashPath);
}
}));
9 changes: 5 additions & 4 deletions test/parallel/test-child-process-spawnsync-shell.js
Expand Up @@ -54,11 +54,12 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz');

function test(testPlatform, shell, shellOutput) {
platform = testPlatform;

const isCmd = /^(?:.*\\)?cmd(?:\.exe)?$/i.test(shellOutput);
const cmd = 'not_a_real_command';
const shellFlags = platform === 'win32' ? ['/d', '/s', '/c'] : ['-c'];
const outputCmd = platform === 'win32' ? `"${cmd}"` : cmd;
const windowsVerbatim = platform === 'win32' ? true : undefined;

const shellFlags = isCmd ? ['/d', '/s', '/c'] : ['-c'];
const outputCmd = isCmd ? `"${cmd}"` : cmd;
const windowsVerbatim = isCmd ? true : undefined;
internalCp.spawnSync = common.mustCall(function(opts) {
assert.strictEqual(opts.file, shellOutput);
assert.deepStrictEqual(opts.args,
Expand Down