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

VT100 escape sequences appearing on Windows 10 Console #3897

Merged
merged 4 commits into from
Mar 31, 2021

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 27, 2019

During opam init, things proceed nicely until a second or so after the repository tarball is downloaded when the "Processing" message suddenly appears as:

<><> Fetching repository information ><><><><><><><><><><><><><><><><><><><><><>
�[K[�[32mdefault�[0m] Initialisedlt�[0m: http]
�[KProcessing  1/1:
�[36m<><>�[0m �[01mRequired setup - please read�[0m
�[36m<><><><><><><><><><><><><><><><><><><><><><><>�[0m   

The problem is that processes invoked by opam gain stdin and therefore the Console and clear the Console mode (in particular, Cygwin processes seem to do this). They're running in the background, so it doesn't matter whether they behave nicely and restore the console mode on termination, because the message may well be displayed while they're running.

This PR offers two "fixes":

  • firstly the OpamConsole mode is altered to force-set the mode on every print (this is ugly, but not much more ugly than what was there before)
  • secondly the default for allow_stdin is declared false on Windows, and a couple of places where allow_stdin should clearly be false (cygpath invocation and the download command) are set

Either of these on their own is sufficient to fix the Windows problem.

@dra27 dra27 mentioned this pull request Jul 2, 2019
@XVilka
Copy link
Contributor

XVilka commented Nov 28, 2019

Will be this a part of 2.0.6? Seems a reasonable user experience bug fix.

@rjbou rjbou requested a review from AltGr January 20, 2020 16:59
Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Looks good to me. Closing stdin might cause problems with some commands on Linux, but I think here it will be OK; and anyway we will have time to check those.

@dra27
Copy link
Member Author

dra27 commented Jan 27, 2020

This isn’t ready to go - the CI failures are relevant

@dra27 dra27 self-assigned this Mar 12, 2021
@dra27 dra27 added this to the 2.1.0~rc milestone Mar 12, 2021
@dra27
Copy link
Member Author

dra27 commented Mar 25, 2021

I don't intend merging 890c297 (the stdin change) in 2.1.

Subprocesses which gain access to the Console may reset it while running
in the background.
@dra27
Copy link
Member Author

dra27 commented Mar 26, 2021

Good to go with CI!

Copy link
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

LGTM

@dra27 dra27 merged commit 074df6b into ocaml:master Mar 31, 2021
@dra27
Copy link
Member Author

dra27 commented Mar 31, 2021

�[32mThank you!�[0m 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants