Skip to content

child-process: support any shells on Windows #21943

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.

Sign up for GitHub

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

Closed
tkamenoko wants to merge 16 commits into from
Closed

child-process: support any shells on Windows #21943

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b42be28
child-process: fix switches for alternative shells on Windows
tkamenoko Jul 21, 2018
e9863e6
child-process: fix lint errors
tkamenoko Jul 21, 2018
6f3bccc
test: add exec-any-shells test
tkamenoko Jul 21, 2018
6b9b090
test: fix exec-any-shell test for powershell
tkamenoko Jul 23, 2018
1dcea34
child-process: enable to accept full path that contains spaces
tkamenoko Jul 23, 2018
c7163f8
child-process: undo unrelated style changes
tkamenoko Jul 24, 2018
a314340
test: remove hard coding path
tkamenoko Jul 26, 2018
ce7999a
test: fix spawnsync-shell to check correct options on Windows
tkamenoko Jul 26, 2018
097c905
child-process: set windowsVerbatimArguments true for any shell
tkamenoko Aug 2, 2018
a4122b7
child-process: use case-insensitive comparison to check shell name
tkamenoko Aug 2, 2018
27ba48f
test: add testcases to ensure uppercase shell name and complex commands
tkamenoko Aug 2, 2018
f93c796
test: fix to accept multiple lines output of `where bash`
tkamenoko Aug 2, 2018
ade01d9
test: revert windowsVerbatim flag, fix `isCmd` check in spawnsync-shell
tkamenoko Aug 2, 2018
5a2c130
doc: update shell requirements in child_process
tkamenoko Aug 7, 2018
e2192a3
fixup: windowsVerbatimArguments
joaocgreis Aug 14, 2018
7a10600
fixup: don't exhaust exec buffer
joaocgreis Sep 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup: windowsVerbatimArguments
  • Loading branch information
@joaocgreis
joaocgreis committed Sep 1, 2018
commit e2192a324fc178d803c112ce52b5265df8c030ed
4 changes: 2 additions & 2 deletions doc/api/child_process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
2 changes: 1 addition & 1 deletion lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,10 +478,10 @@ function normalizeSpawnArguments(file, args, options) {
// '/d /s /c' is used only for cmd.exe.
if (/^(?:.*\\)?cmd(?:\.exe)?$/i.test(file)) {
Copy link
Copy Markdown
Contributor

@refack refack Aug 20, 2018

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
Copy Markdown
Contributor

@refack refack Aug 20, 2018

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;
Copy link
Copy Markdown
Member

@joaocgreis joaocgreis Jul 31, 2018

Choose a reason for hiding this comment

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

windowsVerbatimArguments should be set out of this if block, to be active for any shell (this is mentioned in the documentation for spawn)

} else {
args = ['-c', command];
}
options.windowsVerbatimArguments = true;
} else {
if (typeof options.shell === 'string')
file = options.shell;
Expand Down
56 changes: 42 additions & 14 deletions test/parallel/test-child-process-exec-any-shells-windows.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,63 @@
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 }, (error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes('foo') && stdout.includes('bar'));
});
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');

cp.exec('where bash', (error, stdout) => {
if (error) {
return;
}
test(stdout.split(/[\r\n]+/g)[0].trim());
});

// Test PowerShell
test('powershell');
Copy link
Copy Markdown
Member

@joaocgreis joaocgreis Jul 31, 2018

Choose a reason for hiding this comment

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

Can you also include a more complex PowerShell test (with quotes and pipes)? Something like

cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`,
        { shell: 'PowerShell' }, (error, stdout, stderror) => {
  assert.ok(!error && !stderror);
  assert.ok(stdout.includes('test-child-process-exec-any-shells-windows.js'));
});

testCopy('powershell.exe',
`${system32}\\WindowsPowerShell\\v1.0\\powershell.exe`);
cp.exec(`Get-ChildItem "${__dirname}" | Select-Object -Property Name`,
{ shell: 'PowerShell' }, (error, stdout, stderror) => {
{ shell: 'PowerShell' }, common.mustCall((error, stdout, stderror) => {
assert.ok(!error && !stderror);
assert.ok(stdout.includes(
'test-child-process-exec-any-shells-windows.js'));
});
}));

// 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);
}
}));
2 changes: 1 addition & 1 deletion test/parallel/test-child-process-spawnsync-shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz');

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