child-process: support any shells on Windows #21943

tkamenoko · 2018-07-23T12:11:00Z

On Windows, child_process methods like exec supported only cmd.exe as the shell. This change enables these methods to accept any shells using -c switch like Unix shells.

Fixes: #21905

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax · 2018-07-23T12:43:03Z

lib/child_process.js

@@ -399,7 +400,7 @@ function normalizeSpawnArguments(file, args, options) {
  if (Array.isArray(args)) {
    args = args.slice(0);
  } else if (args !== undefined &&
-             (args === null || typeof args !== 'object')) {
+    (args === null || typeof args !== 'object')) {


Can you undo unrelated style changes? They make review harder, break git blame, and tend to induce merge conflicts.

targos · 2018-07-24T09:15:58Z

test/parallel/test-child-process-exec-any-shells-windows.js

+test('cmd.exe');
+test('C:\\WINDOWS\\system32\\cmd.exe');
+test('powershell');
+test('C:\\Program Files\\Git\\bin\\bash.exe');


Is it a build requirement to have git bash installed on Windows?

Yes, git bash is one of prerequisites.

Basic Unix tools required for some tests, Git for Windows includes Git Bash and tools which can be included in the global PATH.

bash for windows is not a build requirement, but ATM it's a requirement for running the test suite.

node/BUILDING.md

Lines 265 to 267 in 5384570

* Basic Unix tools required for some tests,

[Git for Windows](http://git-scm.com/download/win) includes Git Bash

and tools which can be included in the global `PATH`.

However it is not guaranteed to be at C:\\Program Files\\Git. This could be done by running where bash, and also skipping if it not found.

We should not be assuming any paths, e.g. the SystemDrive might not be C:.

Oh, I had a misunderstanding.

Well, can I use exec in the exec test? If not, is there a workaround?

// Replace the test L23 to the following. cp.exec('where bash', (error, stdout) => { if (error) { return; }; test(stdout.trim()); });

Sure, or execSync.

targos · 2018-07-24T09:33:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15992/

refack · 2018-07-24T13:39:59Z

test/parallel/test-child-process-exec-any-shells-windows.js

+test('cmd.exe');
+test('C:\\WINDOWS\\system32\\cmd.exe');
+test('powershell');
+test('C:\\Program Files\\Git\\bin\\bash.exe');


bash for windows is not a build requirement, but ATM it's a requirement for running the test suite.

node/BUILDING.md

Lines 265 to 267 in 5384570

* Basic Unix tools required for some tests,

[Git for Windows](http://git-scm.com/download/win) includes Git Bash

and tools which can be included in the global `PATH`.

However it is not guaranteed to be at C:\\Program Files\\Git. This could be done by running where bash, and also skipping if it not found.

tkamenoko · 2018-07-25T10:22:49Z

node/test/parallel/test-child-process-spawnsync-shell.js

Lines 59 to 61 in f5a2167

    
           const shellFlags = platform === 'win32' ? ['/d', '/s', '/c'] : ['-c']; 
        
           const outputCmd = platform === 'win32' ? `"${cmd}"` : cmd; 
        
           const windowsVerbatim = platform === 'win32' ? true : undefined;

This test failed because it expects to use options like /d /s /c for any shells on Windows, but this PR changes this behavior. Is it OK to fix the test?

gireeshpunathil · 2018-07-25T13:56:12Z

yes, absolutely!

tkamenoko · 2018-07-27T06:34:03Z

Why did the test fail? I can't reproduce the error on my local repository.

not ok 17 async-hooks/test-fseventwrap

duration_ms: 0.176
severity: fail
exitcode: 1
stack: |-
internal/fs/watchers.js:167
throw error;
^

Error: ENOSPC: no space left on device, watch '/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js'
    at FSWatcher.start (internal/fs/watchers.js:161:26)
    at Object.watch (fs.js:1235:11)
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/async-hooks/test-fseventwrap.js:16:20)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:266:19)

targos · 2018-07-27T06:56:37Z

@tkamenoko Don't worry about this one. It happens often on Travis

tkamenoko · 2018-07-27T08:54:55Z

@targos Thanks.
It seems that the other js tests than test-fseventwrap passed. What should I do?

gireeshpunathil · 2018-07-27T13:41:51Z

started one more CI , to be certain.
CI: https://ci.nodejs.org/job/node-test-pull-request/16030/

richardlau · 2018-07-27T16:22:37Z

Does any of the documentation need updating?

vsemozhetbyt · 2018-07-27T16:44:51Z

Maybe this fragment?

https://github.com/nodejs/node/blame/449f73b7ffd60e7dd140b360e4b53dc2428f4aef/doc/api/child_process.md#L1375-L1376

tkamenoko · 2018-07-28T08:20:20Z

Are http2 tests flaky?
https://ci.nodejs.org/job/node-test-binary-windows/18779/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=1/tapResults/

parallel/test-http2-pipe failed on win2008r2. It passed on my local (win10 64bit), but parallel/test-http2-client-upload failed instead.

They both got the same error:

ExperimentalWarning: The http2 module is an experimental API.

joaocgreis

@tkamenoko thanks for working on this! I left a few comments that I believe should be addressed, but other than that this is looking good.

I noticed you are adding commits with descriptions for your changes. Usually when we land PRs we squash into only one commit (or more in some cases). In this case, we can keep the title and description of the first commit, so I want to make sure you're OK with that. This can happen only when landing, having multiple commits here in the PR is great if we ever have to come back and understand where something came from.

This PR changes the behavior described in https://github.com/nodejs/node/blob/a4ce449bb7b0e2f9edba75baf2a2f652a6750454/doc/api/child_process.md#shell-requirements . Since child_process is marked as stable, this will have to be semver major, and that section has to be adjusted to reflect these changes. Let me know if you'd like me to suggest some concrete text.

joaocgreis · 2018-07-31T02:32:00Z

lib/child_process.js

+      // '/d /s /c' is used only for cmd.exe.
+      if (file.endsWith('cmd.exe') || file.endsWith('cmd')) {
+        args = ['/d', '/s', '/c', `"${command}"`];
+        options.windowsVerbatimArguments = true;


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

joaocgreis · 2018-07-31T02:32:50Z

lib/child_process.js

-      args = ['/d', '/s', '/c', `"${command}"`];
-      options.windowsVerbatimArguments = true;
+      // '/d /s /c' is used only for cmd.exe.
+      if (file.endsWith('cmd.exe') || file.endsWith('cmd')) {


String comparison should be case insensitive

joaocgreis · 2018-07-31T03:11:51Z

test/parallel/test-child-process-exec-any-shells-windows.js

+  if (error) {
+    return;
+  }
+  test(stdout.trim());


The output of where can have more than one line (in my machine I have bash from git and WSL). There is an example of using where here, but it can probably be simplified for this case.

joaocgreis · 2018-07-31T03:33:29Z

test/parallel/test-child-process-exec-any-shells-windows.js

+
+test('cmd');
+test('cmd.exe');
+test('powershell');


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')); });

joaocgreis · 2018-07-31T04:18:56Z

@nodejs/platform-windows I like this as is, but when this lands people will start depending on this. So, now is a probably good time to discuss: do we want to have an explicit PowerShell case like the one this creates for CMD? Like CMD uses /D (#8063), PowerShell could have -NoProfile. And perhaps it would be useful to have -ExecutionPolicy Unrestricted to easily execute scripts.

tkamenoko · 2018-07-31T08:00:55Z

@joaocgreis I have 2 questions just to make sure:

1: I no longer need to write long descriptions when adding commits, right? Or, should I write descriptions?

2:

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

Do you mean that the code should be like this?

      if (file.endsWith('cmd.exe') || file.endsWith('cmd')) {// fix later
        args = ['/d', '/s', '/c', `"${command}"`];
      } else {
        args = ['-c', command];
      }
      options.windowsVerbatimArguments = true;

Thanks.

joaocgreis · 2018-07-31T18:43:54Z

@tkamenoko

1: no need to write descriptions, just needs to be clear to the person landing this what to do with the commit. I usually write something like fixup: small description. There is no established procedure around this, so as long as it's clear it's good.

2: yes, that looks correct to me.

bzoz · 2018-08-01T15:39:09Z

@tkamenoko can you make this more general? Not all shells will use -c as command, IMHO we should pass those extra shell args as an additional spawn option.

tkamenoko · 2018-08-02T16:34:32Z

@bzoz Sorry, I can't decide by myself.

This PR changes Windows shell requirements to that like Unix, except for cmd.exe.
https://github.com/nodejs/node/blob/a4ce449bb7b0e2f9edba75baf2a2f652a6750454/doc/api/child_process.md#shell-requirements

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'.

Your suggestion is interesting, but I don't know how to implement it without large fix. We need to discuss whether to accept shells that do not use -c. If you know shells like that, it will be helpful to get agreement.

joaocgreis · 2018-08-06T11:50:52Z

@bzoz I think of exec as a way to run a line as if typed on the specified shell, it should know what arguments to use for each shell. Having an option to specify arguments would be ok, but I think it can be done in another PR if anyone is interested. Or do you think this PR should block on that? Note that quoting of the command depends on the arguments.

@tkamenoko your update looks good, but I'm having issues running the test locally. Please let me investigate.

The documentation update is still needed in https://github.com/nodejs/node/blob/a4ce449bb7b0e2f9edba75baf2a2f652a6750454/doc/api/child_process.md#shell-requirements . I suggest something like this:

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.

bzoz · 2018-08-06T14:18:59Z

The shell argument thingy can be done in another PR, this PR can land as it is.

joaocgreis · 2018-08-24T23:01:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16742/

joaocgreis · 2018-08-25T02:02:14Z

Test failure is related: the full listing of our parallel folder is too big for the exec buffer.

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

Modify indent, and replace doublequote to singlequote.

This test ensure that child_process.exec can work with any shells. Giving a shell name if $path is defined, or full path is given. Testing with cmd, powershell, and git-bash, but the test fails when testing with git-bash. Git-bash is usually placed at 'C:\Program Files\Git\bin\', and it has a space that cause the failure.

When giving 'echo foo bar' to powershell, the result contains a new line like: foo bar This commit enables to test with poweshell correctly.

Before this commit, if given path contained spaces, like 'Program Files', exec failed to find a shell. 'options.windowsVerbatimArguments' is used for cmd.exe only, and spaces are escaped correctly.

Auto-formatting cause unnecessary indentations. These style changes are removed.

test-child-process-any-shell uses `where` before testing with bash to remove hard coding path.

This test expected to use cmd.exe options even if the shell was powershell. This commit fixes to check correct options for shells other than cmd.exe.

joaocgreis · 2018-09-01T22:56:29Z

@tkamenoko I pushed a commit to address the test failure. Feel free to change it or remove it if you don't agree with it.

Rebased and new CI: https://ci.nodejs.org/job/node-test-pull-request/16929/

joaocgreis · 2018-09-05T19:53:22Z

Landed in af883e1

Thanks for your contribution @tkamenoko !

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: #21905 PR-URL: #21943 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

ackvf · 2018-10-08T22:24:12Z

In which version of node was this released?

richardlau · 2018-10-08T22:41:48Z

In which version of node was this released?

@ackvf It hasn't. This is semver-major so will be in Node.js 11.

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jul 23, 2018

addaleax added the windows Issues and PRs related to the Windows platform. label Jul 23, 2018

addaleax reviewed Jul 23, 2018

View reviewed changes

jasnell approved these changes Jul 23, 2018

View reviewed changes

targos reviewed Jul 24, 2018

View reviewed changes

refack suggested changes Jul 24, 2018

View reviewed changes

tkamenoko force-pushed the child-process-for-pwsh branch from 511b977 to 17ded87 Compare July 26, 2018 13:47

joaocgreis suggested changes Jul 31, 2018

View reviewed changes

joaocgreis added semver-major PRs that contain breaking changes and should be released in the next major version. dont-land-on-v6.x labels Jul 31, 2018

targos removed the dont-land-on-v10.x label Jul 31, 2018

joaocgreis force-pushed the child-process-for-pwsh branch from 5c3943a to 3a036cc Compare August 24, 2018 22:58

tkamenoko and others added 15 commits September 1, 2018 23:32

child-process: fix lint errors

e9863e6

Modify indent, and replace doublequote to singlequote.

test: fix exec-any-shell test for powershell

6b9b090

When giving 'echo foo bar' to powershell, the result contains a new line like: foo bar This commit enables to test with poweshell correctly.

child-process: enable to accept full path that contains spaces

1dcea34

Before this commit, if given path contained spaces, like 'Program Files', exec failed to find a shell. 'options.windowsVerbatimArguments' is used for cmd.exe only, and spaces are escaped correctly.

child-process: undo unrelated style changes

c7163f8

Auto-formatting cause unnecessary indentations. These style changes are removed.

test: remove hard coding path

a314340

test-child-process-any-shell uses `where` before testing with bash to remove hard coding path.

test: fix spawnsync-shell to check correct options on Windows

ce7999a

This test expected to use cmd.exe options even if the shell was powershell. This commit fixes to check correct options for shells other than cmd.exe.

child-process: set windowsVerbatimArguments true for any shell

097c905

child-process: use case-insensitive comparison to check shell name

a4122b7

test: add testcases to ensure uppercase shell name and complex commands

27ba48f

test: fix to accept multiple lines output of where bash

f93c796

test: revert windowsVerbatim flag, fix isCmd check in spawnsync-shell

ade01d9

doc: update shell requirements in child_process

5a2c130

fixup: windowsVerbatimArguments

e2192a3

joaocgreis force-pushed the child-process-for-pwsh branch from a624cb7 to 60437d2 Compare September 1, 2018 22:36

fixup: don't exhaust exec buffer

7a10600

joaocgreis force-pushed the child-process-for-pwsh branch from 60437d2 to 7a10600 Compare September 1, 2018 22:47

joaocgreis closed this Sep 5, 2018

This was referenced Sep 21, 2018

Yarn run command doesn't wok in 1.6.0 and 1.7.0 yarnpkg/yarn#6086

Closed

yarn run doesn't work in Git Bash due to strange path modification yarnpkg/yarn#5717

Open

richardlau mentioned this pull request Apr 6, 2020

build: add build from tarball #32129

Closed

2 tasks

	* Basic Unix tools required for some tests,
	[Git for Windows](http://git-scm.com/download/win) includes Git Bash
	and tools which can be included in the global `PATH`.

child-process: support any shells on Windows #21943

child-process: support any shells on Windows #21943

Conversation

tkamenoko commented Jul 23, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

targos Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jul 24, 2018

Choose a reason for hiding this comment

tkamenoko commented Jul 25, 2018

gireeshpunathil commented Jul 25, 2018

tkamenoko commented Jul 27, 2018

not ok 17 async-hooks/test-fseventwrap

targos commented Jul 27, 2018

tkamenoko commented Jul 27, 2018

gireeshpunathil commented Jul 27, 2018

richardlau commented Jul 27, 2018

vsemozhetbyt commented Jul 27, 2018

tkamenoko commented Jul 28, 2018

joaocgreis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaocgreis commented Jul 31, 2018

tkamenoko commented Jul 31, 2018

joaocgreis commented Jul 31, 2018

bzoz commented Aug 1, 2018

tkamenoko commented Aug 2, 2018

joaocgreis commented Aug 6, 2018

bzoz commented Aug 6, 2018

joaocgreis commented Aug 24, 2018

joaocgreis commented Aug 25, 2018

joaocgreis commented Sep 1, 2018

joaocgreis commented Sep 5, 2018

ackvf commented Oct 8, 2018

richardlau commented Oct 8, 2018

tkamenoko commented Jul 23, 2018 •

edited

Loading

targos Jul 24, 2018 •

edited

Loading