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

repl: disable Ctrl+C support on win32 for now #7977

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 4, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

Description of change

Disable Windows support for interrupting REPL commands using Ctrl+C by default, because the switches from console raw mode and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is very likely not related to this specific feature.

Ref: #7837

/cc @joaocgreis

Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: nodejs#7837
@addaleax addaleax added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. labels Aug 4, 2016
@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Oy, this is unfortunate but LGTM as a temporary measure.

/cc @nodejs/platform-windows

@seishun
Copy link
Contributor

seishun commented Aug 4, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2016

LGTM

@joaocgreis
Copy link
Member

LGTM, thanks for the env var!

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

oh! mentioning the env var reminded me... is the intent to keep the env var undocumented and temporary as well? If not, then this should also add documentation to printHelp and docs.

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@jasnell No, keeping it uncodumented is exactly the plan (or at least what I am having in mind). I thought about getting fancy with underscores or something, but that just seemed weird for env vars.

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

+1 awesome. That's what I suspected but wanted to clarify. It may be worthwhile adding a code comment to that effect in case someone else comes across it and wonders.

@addaleax
Copy link
Member Author

addaleax commented Aug 4, 2016

@jasnell Done! :)

@jasnell
Copy link
Member

jasnell commented Aug 4, 2016

Thank you! :-)

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

New CI run just to be safe: https://ci.nodejs.org/job/node-test-pull-request/3571/

jasnell pushed a commit that referenced this pull request Aug 8, 2016
Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: #7837

PR-URL: #7977
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: João Reis <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

Landed in f59b888!

@jasnell jasnell closed this Aug 8, 2016
@addaleax addaleax deleted the disable-ctrlc-windows branch August 8, 2016 16:45
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Disable Windows support for interrupting REPL commands using
Ctrl+C by default, because the switches from console raw mode
and back have been interfering with printing the results of
evaluated expressions.

This is a temporary measure, since the underlying problem is
very likely not related to this specific feature.

Ref: #7837

PR-URL: #7977
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Nikolai Vavilov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: João Reis <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@MylesBorins
Copy link
Contributor

@addaleax does this need to be backported?

@addaleax
Copy link
Member Author

No. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants