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

Add child_process.spawnShell #1009

Closed
storzinc opened this issue Feb 28, 2015 · 19 comments
Closed

Add child_process.spawnShell #1009

storzinc opened this issue Feb 28, 2015 · 19 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. windows Issues and PRs related to the Windows platform.

Comments

@storzinc
Copy link

In the child_process module currently there is two flavors of exec function: exec and execFile, which is good, but for spawn, there is only one version, would've been great to have something like child_process.spawnShell. Currently I use this code to emulate it:

child_process.spawn('cmd', ['/s', '/c', command], {windowsVerbatimArguments:true})

but it is platform specific and requires the use of undocumented flag windowsVerbatimArguments

@tellnes tellnes added child_process Issues and PRs related to the child_process subsystem. discuss Issues opened for discussions and feedbacks. labels Mar 1, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Mar 1, 2015

Maybe I misunderstand the question, but isn't that what exec() is for?

@tellnes
Copy link
Contributor

tellnes commented Mar 1, 2015

If I understood the question correctly, what he wants a way to spawn a shell without running any commands.

I can't see the a big use case for this since you anyway need to run different commands in windows and unix. What I'm saying is that you probably need the if which this will remove from your code anyway.

The only benefit would be that you don't need to use the undocumented windowsVerbatimArguments flag. Maybe a better solution is to document that?

@storzinc
Copy link
Author

storzinc commented Mar 1, 2015

Actually I want to execute a shell command, but with the ability for spawned shell to inherit stdout from the current process. It is possible with spawn but not possible with exec.

Documenting windowsVerbatimArguments will also solve this issue for me, but I still think it will be better not to add platform specific flags if possible, and separate function will be better.

@sam-github
Copy link
Contributor

It's a big hole in the child_process API. You can't spawn node scripts, like npm, on windows. Ouch.

@storzinc I suggest you write an npm module that implements spawn as you'd like it to be, so it's API can get banged on and confirmed robust. I'd use it, and be happy to collaborate a bit on it, but I don't have time right now to write it.

At that point, with a solid API and implementation, I'd +1 an attempt to get it into node. As you say, we have exec-with-shell and exec-direct, why not a spawn-with-shell, as well as spawn-direct.

@piscisaureus
Copy link
Contributor

spawn('foo | bar > baz', { shell: true }) maybe?

@sam-github
Copy link
Contributor

I'd be OK with the above. I'd sure like to deprecate exec/execFile so that it followed the same pattern.

@piscisaureus do you think its worth PRing directly to core, or out of core first as an npm module?

@piscisaureus
Copy link
Contributor

I'd sure like to deprecate exec/execFile so that it followed the same pattern.

But exec(File) buffers stdout. You think that's not useful?

@tellnes
Copy link
Contributor

tellnes commented Mar 3, 2015

I'd sure like to deprecate exec/execFile so that it followed the same pattern.

But exec(File) buffers stdout. You think that's not useful?

That is useful, so please don't deprecate it.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 3, 2015

I assumed he meant using the {shell: true} pattern instead of having two separate methods.

@sam-github
Copy link
Contributor

Indeed. With @piscisaureus 's suggestion, which I like, we would have:

  • spawn: defaults to shell: false
  • exec: defaults to shell: true
  • execFile: identical to exec, but defaults to shell: false

I'd prefer an API where spawn and exec both default to shell: false, but I doubt that would be accepted, too incompatible. So, the fact that exec and spawn have opposite defaults for the shell option will plague us forever.

@storzinc
Copy link
Author

storzinc commented Mar 7, 2015

I have made my npm module as suggested:
https://www.npmjs.com/package/child_process2
It is just repackaged child_process.js from iojs sources with added support for the shell option

@brendanashworth
Copy link
Contributor

Since this is now available in npm, I will close this. Thank you!

@storzinc
Copy link
Author

storzinc commented Jul 1, 2015

@brendanashworth I was actually hoping that it will be integrated in iojs... As for my npm module - it is relying upon iojs internals (it is just modified child_process.js from iojs itself), and as such can not be used for anything serious.

@brendanashworth brendanashworth added feature request Issues that request new features to be added to Node.js. and removed discuss Issues opened for discussions and feedbacks. labels Jul 1, 2015
@brendanashworth
Copy link
Contributor

@storzinc I'll reopen - we usually have a policy of not adding to core what we have in userland, but if you'd like it to be in core that can be discussed. Would you be open to us exposing the functionality you need while keeping it as an npm module?

@sam-github sam-github added the windows Issues and PRs related to the Windows platform. label Aug 6, 2015
@sam-github
Copy link
Contributor

@storzinc your child_process2 package doesn't have any metadata to point to its github repo, so its not very useful, you can't see the source on npmjs.org.

/to @orangemocha, one of the things we were just talking about. I labelled it Windows, because it is particularly useful for windows, though spawning with a shell can be done on Unixen as well.

@storzinc
Copy link
Author

storzinc commented Aug 7, 2015

@sam-github you can get tarball with sources directly from npmjs.org:
http://registry.npmjs.org/child_process2/-/child_process2-2.0.1.tgz
it is based on the io.js-1.2.0 sources, but I can update it to the current version if this will help.

@sam-github
Copy link
Contributor

@storzinc, I know how to download npm tarballs. That is a difficult way to browse source, particular source that you wrote to accompany this issue. Its your call, but I suggest you make your source easier to view if you hope people will view it.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2015

This issue kind of died. Are we going to work to port @storzinc's changes into core? Otherwise, I think this can be closed.

@sam-github
Copy link
Contributor

Depends: is it a gaping hole in the child_process API? Yes. But does that mean the issue needs to stay open without someone actively working on trying to get that hole fixed? I'm not sure, is that useful? The issue could be closed, still be in the record, and the conversation can continue.

cjihrig added a commit to cjihrig/node that referenced this issue Jan 27, 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.

Fixes: nodejs#1009
PR-URL: nodejs#4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Feb 10, 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.

Fixes: #1009
PR-URL: #4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 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.

Fixes: nodejs#1009
PR-URL: nodejs#4598
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue 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 pushed a commit that referenced this issue 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 pushed a commit that referenced this issue 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>
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

6 participants