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

Revert "repl: disable Ctrl+C support on win32 for now" #8645

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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

repl

Description of change

This reverts commit f59b888, since apparently libuv/libuv#1054 fixed the underlying problem that led to temporary disabling. I’d really like Windows users (@seishun? @bzoz?) to check that everything works as expected.

Ref: libuv/libuv#1054
Ref: #7837

@addaleax addaleax added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. labels Sep 18, 2016
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Sep 18, 2016
@seishun
Copy link
Contributor

seishun commented Sep 18, 2016

Too early for that since libuv hasn't been upgraded.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Sep 18, 2016
@addaleax
Copy link
Member Author

@seishun yeah, I realize that you’d have to apply the libuv change as well :)

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once it's time to land this. The libuv update should be in the near future.

Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

@addaleax ... ping :-)

@addaleax
Copy link
Member Author

addaleax commented Oct 6, 2016

This is still blocked … presumably on #8280, but generally whatever libuv update includes the fix.

@jasnell
Copy link
Member

jasnell commented Oct 6, 2016

ok, figured as much, just wanted to check in on it tho :-)

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
@addaleax addaleax added lts-watch-v6.x and removed blocked PRs that are blocked by other issues or PRs. labels Oct 27, 2016
@addaleax
Copy link
Member Author

Rebased now that the libuv update has landed – this should work now.

Copy link
Contributor

@seishun seishun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can't reproduce #7837 anymore.

@addaleax
Copy link
Member Author

@seishun 🎉 Thanks for confirming!

@addaleax
Copy link
Member Author

@addaleax
Copy link
Member Author

Landed in 2e28875

@addaleax addaleax closed this Nov 16, 2016
@addaleax addaleax deleted the revert-repl-ctrl-c-win32 branch November 16, 2016 18:45
addaleax added a commit that referenced this pull request Nov 16, 2016
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

this should be backported when we update libuv to v1.10.x

@MylesBorins MylesBorins mentioned this pull request Nov 18, 2016
2 tasks
@addaleax
Copy link
Member Author

@MylesBorins This should land on v6.x (because the commit that is reverted by this PR is also in v6.x). But together with or after the libuv upgrade, yes.

MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

thanks for the heads up

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
PR-URL: nodejs#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs#7837
PR-URL: nodejs#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

@addaleax it would appear that this shouldn't have landed on v6.x before the libuv upgrade

reverted the revert in 1978911

@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
2 tasks
MylesBorins added a commit to MylesBorins/node that referenced this pull request Mar 29, 2017
Full original message:

  Revert "repl: disable Ctrl+C support on win32 for now"

This reverts commit 1d400ea.

Fixes: nodejs#12085
Refs: nodejs#8645

PR-URL: nodejs#12123
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this pull request Mar 29, 2017
Full original message:

  Revert "repl: disable Ctrl+C support on win32 for now"

The original fix was a stop gap until a libuv update landed.
As the libuv update has not yet landed on v6.x the revert should
not have landed. This commit reverts 1d400ea reapplying the
stopgap fix until we update libuv.

Fixes: #12085
Refs: #8645

PR-URL: #12123
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins added a commit that referenced this pull request Mar 29, 2017
Full original message:

  Revert "repl: disable Ctrl+C support on win32 for now"

The original fix was a stop gap until a libuv update landed.
As the libuv update has not yet landed on v6.x the revert should
not have landed. This commit reverts 1d400ea reapplying the
stopgap fix until we update libuv.

Fixes: #12085
Refs: #8645

PR-URL: #12123
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: #7837
PR-URL: #8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
This reverts commit f59b888
now that the libuv update containing the proper fix has
landed in 63243bc.

Ref: libuv/libuv#1054
Ref: nodejs/node#7837
PR-URL: nodejs/node#8645
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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.

7 participants