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: add shell option to spawn() #4598

Merged
merged 1 commit into from Jan 27, 2016
Merged

child_process: add shell option to spawn() #4598

merged 1 commit into from Jan 27, 2016

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 9, 2016

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Closes #1009

if (options && options.shell)
file = options.shell;
// Make a shallow copy so we don't clobber the user's options object
options = util._extend({}, options);

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 9, 2016
Member

#4593 is about deprecating util._extend(). Maybe use Object.assign() here?

EDIT: Just a suggestion, feel free to ignore.

This comment has been minimized.

@cjihrig

cjihrig Jan 11, 2016
Author Contributor

I'd prefer to use Object.assign(). I was waiting to see how #4593 played out, but the reaction seems to be good, so I'll replace this.


if (options && options.shell)
file = options.shell;
// Make a shallow copy so we don't clobber the user's options object

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 9, 2016
Member

s/object/object./

@@ -109,7 +97,6 @@ function normalizeExecArgs(command /*, options, callback*/) {
exports.exec = function(command /*, options, callback*/) {
var opts = normalizeExecArgs.apply(null, arguments);
return exports.execFile(opts.file,
opts.args,

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 9, 2016
Member

How does this work? execFile() ends up calling spawn() but I can't figure out where opts.args is used as the arguments array.

This comment has been minimized.

@cjihrig

cjihrig Jan 11, 2016
Author Contributor

I'm not sure I understand your question. The args to execFile() is optional. Before this change, args would contain the arguments to the shell and the command. With this change, that happens in normalizeSpawnArguments().

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Here is what confuses me:

  1. exec() calls execFile() without an args argument.
  2. execFile() calls spawn() with an empty array as its args argument.
  3. spawn() calls normalizeSpawnArguments() with that empty array as its args argument.
  4. normalizeSpawnArguments() returns that empty array in an options object.
  5. The opts.args from that object is passed to child.spawn().

IOW, I get the impression that the opts.args from normalizeExecArgs() is lost. I assume that's not the case but I don't understand why not.

This comment has been minimized.

@cjihrig

cjihrig Jan 26, 2016
Author Contributor

Prior to this change, args would have been something like ['-c', command]. That would have been sent through execFile(), spawn(), etc.

With this change, normalizeExecArgs() sets shell in options instead. This is passed through until normalizeSpawnArguments(), where the same ['-c', command] is created and passed to child.spawn().

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Alright, that makes sense. Objection withdrawn.

const cmd = common.isWindows ? 'echo bar | more' : 'echo bar | cat';
const command = cp.spawnSync(cmd, {shell: true});

assert.strictEqual(command.stdout.toString().trim(), 'bar');

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 9, 2016
Member

Can you add one or two more tests to verify that the environment is inherited correctly?

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 11, 2016

@bnoordhuis I've addressed your comments.

@jasnell
Copy link
Member

@jasnell jasnell commented Jan 11, 2016

LGTM

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 11, 2016

@cjihrig cjihrig force-pushed the cjihrig:shell branch Jan 11, 2016
@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 11, 2016

CI is all green, but I'd like sign off from @bnoordhuis

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 18, 2016

@bnoordhuis hope you're feeling better. Ping for a review.

@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 26, 2016

@bnoordhuis sorry, one more ping.

options = Object.assign({}, options);

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

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Shouldn't double quotes be escaped on Windows?

This comment has been minimized.

@cjihrig

cjihrig Jan 26, 2016
Author Contributor

Probably. I'll add a test for it too.

This comment has been minimized.

@cjihrig

cjihrig Jan 26, 2016
Author Contributor

Actually, maybe not:

C:\Users\Colin>cmd.exe /s /c "echo "foo""
"foo"

C:\Users\Colin>cmd.exe /s /c "echo \"foo\""
\"foo\"

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Huh, I wonder what cmd.exe's algorithm for that is. Counting quotes?

This comment has been minimized.

@cjihrig

cjihrig Jan 27, 2016
Author Contributor

Ah, looks like the /s enables that behavior - http://stackoverflow.com/questions/9866962/what-is-cmd-s-for

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 27, 2016
Member

Learned something new today. Thanks.

var ret = spawnSync(opts.file, opts.args, opts.options);
ret.cmd = opts.cmd;
var ret = spawnSync(opts.file, opts.options);
ret.cmd = command;

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

I suppose the change on this line is harmless but I don't quite see why you made it. Because the format of opts.cmd may change in the future?

This comment has been minimized.

@cjihrig

cjihrig Jan 26, 2016
Author Contributor

I changed it because opts.cmd came from normalizeExecArgs(), but is removed in this PR.

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Missed that. Carry on.

});
let echoOutput = '';

assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1, 'echo foo']);

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

I think the ] is misplaced here?

}));

// Verify that the environment is properly inherited
const env = cp.spawn(`${process.execPath} -pe process.env.BAZ`, {

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Maybe "${process.execPath}"? This looks like it would fail if there were spaces in the path.

// Verify that passing arguments works
const echo = cp.spawnSync('echo', ['foo'], {shell: true});

assert.strictEqual(echo.args[echo.args.length - 1, 'echo foo']);

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 26, 2016
Member

Misplaced ] again?

This comment has been minimized.

@cjihrig

cjihrig Jan 26, 2016
Author Contributor

Good catch. Oh JavaScript for allowing this. Fixed it up.

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jan 26, 2016

Sorry for the delay. Left some comments.

@cjihrig cjihrig force-pushed the cjihrig:shell branch Jan 26, 2016
@cjihrig cjihrig force-pushed the cjihrig:shell branch Jan 26, 2016
@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Jan 26, 2016

@bnoordhuis, I think I've addressed all of your comments.

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jan 26, 2016

LGTM if the CI is happy.

This commit adds a shell option, to spawn() and spawnSync(). This
option allows child processes to be spawned with or without a
shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Fixes: #1009
PR-URL: #4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Dec 29, 2016

/cc @nodejs/lts
Would we want to include this in a future LTS release for v4.x?

@sam-github
Copy link
Contributor

@sam-github sam-github commented Dec 29, 2016

I'm +1 for back-port, it improves compatibility between the LTS versions.

MylesBorins added a commit that referenced this pull request Jan 13, 2017
This commit adds a shell option, to spawn() and spawnSync(). This
option allows child processes to be spawned with or without a
shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Fixes: #1009
PR-URL: #4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 24, 2017
This commit adds a shell option, to spawn() and spawnSync(). This
option allows child processes to be spawned with or without a
shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Fixes: #1009
PR-URL: #4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins added a commit that referenced this pull request Feb 1, 2017
This commit adds a shell option, to spawn() and spawnSync(). This
option allows child processes to be spawned with or without a
shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Fixes: #1009
PR-URL: #4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Feb 21, 2017
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796
MylesBorins added a commit that referenced this pull request Feb 22, 2017
Notable Changes:

* child_process: add shell option to spawn() (cjihrig)
  #4598
* crypto:
  * add ALPN Support (Shigeki Ohtsu)
    #2564
  * allow adding extra certs to well-known CAs (Sam Roberts)
    #9139
* deps:
  * v8: expose statistics about heap spaces (Ben Ripkens)
    #4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
  #5333
* process:
  * add `externalMemory` to `process` (Fedor Indutny)
    #9587
  * add process.cpuUsage() (Patrick Mueller)
    #10796

PR-URL: #10973
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notable Changes:

    * child_process: add shell option to spawn() (cjihrig)
      nodejs/node#4598
    * crypto:
      * add ALPN Support (Shigeki Ohtsu)
        nodejs/node#2564
      * allow adding extra certs to well-known CAs (Sam Roberts)
        nodejs/node#9139
    * deps:
      * v8: expose statistics about heap spaces (Ben Ripkens)
        nodejs/node#4463
    * fs: add the fs.mkdtemp() function. (Florian MARGAINE)
      nodejs/node#5333
    * process:
      * add `externalMemory` to `process` (Fedor Indutny)
        nodejs/node#9587
      * add process.cpuUsage() (Patrick Mueller)
        nodejs/node#10796

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
hgwood pushed a commit to hgwood/cross-env that referenced this pull request Mar 15, 2017
Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes kentcdodds#77.

BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted
by the shell.
hgwood pushed a commit to hgwood/cross-env that referenced this pull request Mar 15, 2017
Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes kentcdodds#77.

BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted
by the shell.
kentcdodds added a commit to kentcdodds/cross-env that referenced this pull request Mar 15, 2017
* feat(spawn): add support for quoted scripts

Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes #77.


* docs(readme): add gotchas

Add Gotchas paragraph about passing variables to multiple scripts.

* docs(readme): add required node version badge

Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run
cross-env.

* test(spawn): add test for quoted scripts

See #89 (review).


BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.
kentcdodds added a commit to kentcdodds/cross-env that referenced this pull request Mar 31, 2017
* feat(spawn): add support for quoted scripts

Use the shell option of spawn introduced in Node.js 4.8 (see
nodejs/node#4598) to pass the command to the OS shell.
Supersedes #77.


* docs(readme): add gotchas

Add Gotchas paragraph about passing variables to multiple scripts.

* docs(readme): add required node version badge

Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run
cross-env.

* test(spawn): add test for quoted scripts

See #89 (review).


BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants