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: apply hide_console_windows to spawnSync calls #41412

Merged

Conversation

@rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Jan 5, 2022

kHideConsoleWindows should hide console windows for
execSync and spawnSync calls as well. This fix adds on
to #39712.

I have confirmed the fix works downstream using Electron, but haven't yet built it or tested it in node itself.
Refs electron/electron#32340 and microsoft/vscode#138792

@rzhao271 rzhao271 changed the title fix: apply hide_console_windows to spawnSync calls child_process: apply hide_console_windows to spawnSync calls Jan 5, 2022
kHideConsoleWindows should hide console windows for
execSync and spawnSync calls as well. This fix adds on
to nodejs#39712.
@rzhao271 rzhao271 force-pushed the rzhao271/spawnsync-hideconsole branch from f77a197 to 92ea0d3 Jan 5, 2022
@rzhao271 rzhao271 marked this pull request as ready for review Jan 5, 2022
zcbenz
zcbenz approved these changes Jan 6, 2022
Copy link
Contributor

@zcbenz zcbenz left a comment

Thanks for fixing this!

cjihrig
cjihrig approved these changes Jan 6, 2022
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@rzhao271
Copy link
Contributor Author

@rzhao271 rzhao271 commented Feb 9, 2022

Hi, is there anything else needed from me to get this PR merged? Thanks in advance.

@tniessen
Copy link
Member

@tniessen tniessen commented Feb 9, 2022

Hi @rzhao271, I am sorry this fell through. It's been marked as ready for a month, so I'll just restart CI and it should be good to go.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 9, 2022

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 11, 2022

@nodejs-github-bot nodejs-github-bot merged commit 863d13c into nodejs:master Feb 11, 2022
75 checks passed
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Feb 11, 2022

Landed in 863d13c

bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
kHideConsoleWindows should hide console windows for
execSync and spawnSync calls as well. This fix adds on
to nodejs#39712.

PR-URL: nodejs#41412
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bengl added a commit to bengl/node that referenced this issue Feb 21, 2022
kHideConsoleWindows should hide console windows for
execSync and spawnSync calls as well. This fix adds on
to nodejs#39712.

PR-URL: nodejs#41412
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bengl bengl mentioned this pull request Feb 21, 2022
bengl added a commit that referenced this issue Feb 22, 2022
kHideConsoleWindows should hide console windows for
execSync and spawnSync calls as well. This fix adds on
to #39712.

PR-URL: #41412
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants