Skip to content

VT100 escape sequences appearing on Windows 10 Console - #3897

Merged
dra27 merged 4 commits into
ocaml:masterfrom
dra27:console-madness
Mar 31, 2021
Merged

dra27 merged 4 commits into
ocaml:masterfrom
dra27:console-madness

Conversation

@dra27

@dra27 dra27 commented Jun 27, 2019

Copy link
Copy Markdown
Member

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

XVilka commented Nov 28, 2019

Copy link
Copy Markdown
Contributor

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

@AltGr AltGr left a comment

Copy link
Copy Markdown
Member

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

dra27 commented Jan 27, 2020

Copy link
Copy Markdown
Member Author

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

dra27 commented Mar 25, 2021

Copy link
Copy Markdown
Member Author

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

dra27 added 3 commits March 26, 2021 09:28
Subprocesses which gain access to the Console may reset it while running
in the background.
@dra27

dra27 commented Mar 26, 2021

Copy link
Copy Markdown
Member Author

Good to go with CI!

@AltGr AltGr left a comment

Copy link
Copy Markdown
Member

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

dra27 commented Mar 31, 2021

Copy link
Copy Markdown
Member Author

�[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