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

Use the /q flag with cmd.exe in child_process.spawn() #27120

Closed
ehmicky opened this issue Apr 7, 2019 · 8 comments
Closed

Use the /q flag with cmd.exe in child_process.spawn() #27120

ehmicky opened this issue Apr 7, 2019 · 8 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. stale windows Issues and PRs related to the Windows platform.

Comments

@ehmicky
Copy link

ehmicky commented Apr 7, 2019

Is your feature request related to a problem? Please describe.
child_process.spawn() with shell: true on Windows calls cmd.exe /d /s /c.

This makes childProcess.stdout include the prompt and command with Batch files.

Describe the solution you'd like
Add the /q flag.

Alternatives
Adding @echo off to Batch files.

@ZYSzys ZYSzys added 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. labels Apr 7, 2019
@bnoordhuis
Copy link
Member

That seems reasonable. The question is: how likely is it someone files a feature request to turn off that /q flag again?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 8, 2019

The question is: how likely is it someone files a feature request to turn off that /q flag again?

Not being a Windows user, I think the answer is "very likely" because we can't make everyone happy all of the time. I think it's time to start letting flags be passed in.

@refack
Copy link
Contributor

refack commented Apr 8, 2019

This makes childProcess.stdout include the prompt and command with Batch files.

I believe adding a default /q is too opinionated, and cmd-unintuitive. Since cmd (as opposed to POSIX shells) runs with echo on by default, IMO we should keep that behavior.
Adding the classic @echo off at the head of the invoked batch file solves this, as it does for any other shell invoking runtime.
This is opposed to /d (ignore Autorun) /s (strip wrapping " from command line) and /c (invoke and exit) which affect the behavior of the invoked shell, and can not be overridden from a batch file.


running this code:

const { execSync } = require('child_process')
const ret = execSync('t.cmd', { encoding: 'utf8' })
console.log(ret)

for the following t.cmd

echo hello

begets:

D:\code\3party\isomorphic-git>echo hello 
hello

while for this t.cmd

@echo off
echo hello

begets:

hello

@ehmicky
Copy link
Author

ehmicky commented Apr 8, 2019

The comment from @refack makes sense to me. I.e. Windows users might not expect this, and this should be done inside the Batch file instead of outside (in the shell).

One could argue though that the existence of the /q flag itself (as opposed to @echo off) shows that some users might expect to turn on that feature from the shell perspective? For those users, turning that flag on at the moment is not straightforward with child_process.

Granted this is not the default behavior of cmd.exe though, i.e. might not be expected.

@refack
Copy link
Contributor

refack commented Apr 8, 2019

For those users, turning that flag on at the moment is not straightforward with child_process.

As @cjihrig commented, instead of changing the default, we could open it up for configuration, or as a new argument.
(p.s. the problem with configuration is we don't have good scoping, so those tend to be global-per-process. So giving access to changing it from code, is a foot gun, that 3party modules authors can use to shoot the app developers in the foot. Then there's cli args, but that might be overkill)

@himself65
Copy link
Member

yes, I think we can add a new configuration with the default option /d /s /c

and I'm making this thing

@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Feb 25, 2022
@github-actions
Copy link
Contributor

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

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. stale windows Issues and PRs related to the Windows platform.
Projects
Development

Successfully merging a pull request may close this issue.

6 participants