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

Enable tty, vt sequences on Windows #36598

Merged
merged 8 commits into from
Jul 21, 2020
Merged

Enable tty, vt sequences on Windows #36598

merged 8 commits into from
Jul 21, 2020

Conversation

musm
Copy link
Contributor

@musm musm commented Jul 9, 2020

Enable tty, vt sequences on Windows.

Ultimately, we can't control what external processes assume on the console mode when launched within Julia. With the shared console mode that Julia utilizes, this is problematic and the terminal can be left in a corrupted state. As a result, it's safest to reset the mode before each prompt to ensure we enter back into the original console state.
In particular, this fixes problems like that in #26894 (comment).

Currently Julia has disabled vt support, as this can help enhance compatibility. Note, however, we can corrupt the terminal state even under the current status quo, which uniformly disables vt support, and this can be demonstrated through examples. Disabling tty support, has been an annoyance on Windows and has lead to a lot of comments on how the Windows Julia terminal experience is crippled and that this is unfixable since Microsoft has not improved the terminal experience, leading to discussion on other terminal emulators such as mintty. In fact, I think we can support a quality terminal experience on Windows if we carefully enable features that support an improved REPL experience.

This PR in tandem with JuliaLang/libuv#11 enables tty support on Windows.

Here's an example of Julia with these patches using the Windows Terminal:
image

For those that want to try out this PR built with the updated libuv fork that enables tty support on Windows, here's a x86-64 windows installer you can play around with: https://1drv.ms/u/s!AvuMkGVjog3NuaQswzrzMyFP8XlVNw?e=qbv749

closes #31491

@musm musm added the system:windows Affects only Windows label Jul 9, 2020
@davidanthoff
Copy link
Contributor

I just verified that this PR also solves the flickering cursor problem described in #36584. I verified this for the old school Windows console, the new Windows Terminal and the VS Code built-in terminal: all three had the flickering without this PR, with this PR it is gone in all three.

@musm
Copy link
Contributor Author

musm commented Jul 9, 2020

Yes this PR fixes that issue for me, as well!

As it stands, this PR is a stop-gap solution, until libuv has proper pseudo-tty support. Given that we don't have a time frame on when and if such a feature will come, I think something similar to what is proposed in this PR is needed in the meantime. Granted it is a stop-gap solution on already problematic scenario, being that we are forced to use a shared console between child processes, which can have different implicit assumptions on the console state and lead to corruption.

@davidanthoff
Copy link
Contributor

We should probably try this on the oldest officially supported Windows version, just to be sure. This suggests that Windows 10 is the min version for Julia. There are of course very many different Windows 10 versions, and many of the older releases are no longer supported by MS. Looking at this table and assuming that versions that are still supported for enterprise and edu customers are also what Julia officially supports, is 1709 the oldest version that we need to test? Or should we go with 1803 right away, because any build that includes this PR here would come out after 10/13 in any case?

@musm
Copy link
Contributor Author

musm commented Jul 13, 2020

I updated this PR and I actually think it should now be compatible with any Windows version. All that the new code does it reset the terminal mode back to the default mode, in case a shell application modifies it.

@musm
Copy link
Contributor Author

musm commented Jul 13, 2020

For reference here's the example that demonstrates the current way run works is actually broken. This example is courtesy @davidanthoff

echo_cmd = `C:\\Program\ Files\\Git\\usr\\bin\\echo.exe hello`

 @async begin
    sleep(10)
    run(echo_cmd)
 end

run(`wsl`)

Without echo_cmd things, like htop work perfectly in the wsl started by run, but after 10 seconds, the echo_cmd command executes and this completely corrupts the console.

This PR actually improves this pathological test case, because on master executing the above corrupts the console permanently, but with this PR resetting to the default states returns the REPL back to useful state.

@musm
Copy link
Contributor Author

musm commented Jul 13, 2020

So on my end I'm happy with the current state of this RP.

@vtjnash Would love to get your review on this if you have the time 😃.

@musm
Copy link
Contributor Author

musm commented Jul 17, 2020

going to merge in a few days if no objections

ccall(:GetConsoleMode, stdcall, Int32, (Int32, Ptr{Nothing}), hOutput, dwMode)
dwMode[]
end
const default_console_mode = _console_mode()
Copy link
Member

Choose a reason for hiding this comment

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

should not be a build-time constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I changed to be a reference that get's set the first time the REPL is printed at runtime and then is cached perpetually.

#ifdef _OS_WINDOWS_
SetConsoleCP(1252); // ANSI Latin1; Western European (Windows)
#endif

Copy link
Member

Choose a reason for hiding this comment

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

this feels slightly risky and unrelated? (reverts eac525c, but libuv is now unicode-aware, so perhaps that's not as bad now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to remove this or set the console to SetConsoleCP(CP_UTF8) or else the sequences don't get interpreted correctly

Copy link
Member

Choose a reason for hiding this comment

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

CP_UTF8 (65001) is not well supported. That's why we set it to 1252 (It even can break dir: nodejs/node-v0.x-archive#4246 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that comment relevant? It looks like part of the issue was with using a raster front

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at #6795

print("测试")
测试

julia> print("\346\265\213\350\257\225")
测试

With this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like things probably got fixed internally in libuv?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, libuv Is fixed now to bypass this. The issue would be with other shared libraries that assume an ascii-compatible stream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to hear. Is that issue more theoretical than practically relevant (?), I don't know what we could do about this pathological case without regressing the whole system to be dumber because of some library is forcing ascii.

@musm
Copy link
Contributor Author

musm commented Jul 20, 2020

Ok to merge soon?

@StefanKarpinski
Copy link
Member

Unless there are objections or feedback, then yes, please do merge.

@musm
Copy link
Contributor Author

musm commented Jul 21, 2020

Thanks @vtjnash for the review! and also @davidanthoff for working to test this PR and his collaboration on trying to make this a reality.

@musm musm merged commit 34ba868 into JuliaLang:master Jul 21, 2020
@musm musm deleted the winconsole branch July 21, 2020 16:46
@musm musm mentioned this pull request Aug 4, 2020
25 tasks
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
…n Windows (JuliaLang#36598)

On Windows, when launching external processes, we cannot control what assumption they make on the
console mode. We thus forcibly reset the console mode at the start of the prompt to ensure they do
not leave the console mode in a corrupt state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix color support on Windows via VT sequences
4 participants