Skip to content

Changes to support color VT sequences on windows #2

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

Closed
wants to merge 4 commits into from

Conversation

musm
Copy link

@musm musm commented Apr 2, 2019

No description provided.

vtjnash and others added 3 commits August 31, 2018 21:59
This code already had special handling for decoding utf-8
characters correctly for the UCS-2 conhost
(trailing incomplete bytes and characters > 0xFFFF).
Rather than trying to duplicate that, we can simply
delete the duplicate code-path and selectively disable
the parts that do not apply.
@musm
Copy link
Author

musm commented Apr 2, 2019

@vtjnash I don't understand your concern for JuliaLang/julia#27782

Executing shell> cmd /C dir works fine with this patch and remove the codepage setting in libsupport.c in julia

@vtjnash
Copy link
Member

vtjnash commented Apr 2, 2019

Yes, see the linked issues: they are for many exe's that aren't cmd.

@davidanthoff
Copy link

How can one reproduce JuliaLang/julia#26894 or JuliaLang/julia#27782? I used the binary that @musm provided here that includes all his libuv and julia patches, and then I tried the following things, none of which show a problem:

  1. I start julia from the start menu, and press ;ls. That actually doesn't work, because of course ls is not around...
  2. I start git bash, inside git bash I start julia, and then type ;ls. Everything looks good. Same for ;pwd.
  3. I start julia from the start menu, and run ;python. Everything works.
  4. I start julia from the start menu, and run ;powershell. Everything works.

CCing @KristofferC, given that he originally reported JuliaLang/julia#26894. Any suggestion how we might be able to trigger that?

@KristofferC
Copy link
Member

KristofferC commented Apr 2, 2019

Try JuliaLang/julia#26894 (comment) perhaps.

@davidanthoff
Copy link

Yes, now I can reproduce that, thanks!

So, just to record that here, the repo is as follows:

  1. Start the julia binary that @musm provided
  2. In the shell mode, run from the git folder echo blob

Does this only happen for the stuff from Git, or are there any other command line programs that have the same issue?

Here is one clue what is wrong here: when I call GetConsoleMode before I run the shell command, the ENABLE_VIRTUAL_TERMINAL_PROCESSING flag is set. If I run it afterwards (from the corrupted prompt), it is no longer set. So that probably explains why things no longer work: somehow the console is now configured to not interpret VT sequences anymore.

So I guess the real question is how can that happen? Presumably what is happening is that echo.exe is changing the configuration for the console output handle to remove that flag. And this flag is probably per console, not per process, right?

If so, the solution would probably be to do the following whenever a child process is started:

  1. check whether ENABLE_VIRTUAL_TERMINAL_PROCESSING is set
  2. run the child process
  3. set ENABLE_VIRTUAL_TERMINAL_PROCESSING again if it was unset by the child process in the meantime

@davidanthoff
Copy link

There are probably other console specific configurations that a child process in theory could also change and cause trouble? So maybe the right thing to do would be to just call GetConsoleMode before one runs an external program, store all of that, run the external program, and then SetConsoleMode with all the settings that one had obtained previously?

@musm
Copy link
Author

musm commented Apr 3, 2019

Is this then isolated to down to jl_spawn which ends up calling uv_spawn. So again, needs to resolved in libuv?

@musm
Copy link
Author

musm commented Apr 3, 2019

@davidanthoff can you explain again how you reproduced the issue? Are you able to trigger the issue only from git-bash ? I'm unable to trigger the issue from julia within cmd. But running julia from a git bash prompt or running the julia.exe from within wsl I can trigger the issue.

@davidanthoff
Copy link

I installed your compiled version of julia 1.2.0-DEV, then opened the folder where it is installed and double clicked on the julia.exe in there. Then I did this:

image

That reproduces the issue for me.

I then copy-pasted the code that calls GetConsoleMode into that corrupted console, and it shows that the flag is no longer set after I ran the external echo.exe.

@musm musm force-pushed the julia-uv2-1.24.0 branch from f706237 to 461348d Compare April 9, 2019 01:02
@musm
Copy link
Author

musm commented Apr 9, 2019

@vtjnash I updated the branch here by pulling in PR libuv#2129

It seems we need to set uv_tty_set_vterm_state(handle, UV_TTY_AUTODETECT); somewhere in process.c . This is what @davidanthoff is suggesting or his theory of what it is occuring: i.e. the client process changes the console mode.

I'm not sure if we need to handle anything in the julia side with regards to jl_spawn.

Any input would be appreciated.

@musm
Copy link
Author

musm commented Apr 9, 2019

Indeed, it seems that David is correct here

https://stackoverflow.com/questions/52607960/how-can-i-enable-virtual-terminal-processing

The mode setting gets set for the console (i.e. conhost.exe), not the calling process. Specifically it's set for the active console sreen buffer that the process inherited as its StandardOutput. The issue is actually that the parent process is saving the current console mode settings before executing the child and then resets the mode after the child exits. The CMD shell, for example, has worked like this since it was ported as a Windows console application in the early 1990s. In particular, CMD runs programs using the original mode it saved at startup

@musm
Copy link
Author

musm commented Apr 9, 2019

One thing that I am curious about is if this only occurs in the shell> state .

If this is true julia needs to reset the terminal state after we exit the shell> repl state. In other words, I'm not sure that there is anything wrong with the spawn process, but rather the julia terminal failing to reset after switching back to julia repl.

@musm
Copy link
Author

musm commented Apr 9, 2019

Based on the previous comment, the following commit resets the terminal state after every shell command
musm/julia@2feac0b

which does fix the issue that the julia repl gets a corrupted consolemode after spawning certain executables

@vtjnash is this a sufficient patch in julia to resolve the bug you brought to attention?

Test installers (this pr + julia patch shown above): https://www.dropbox.com/s/sutliotmvb9l8k8/julia-6740f8784b-win64.exe?dl=0

EDIT: That doesn't work, since it corrupted the shell mode in a different way...

@musm musm changed the base branch from julia-uv2-1.24.0 to julia-uv2-1.29.1 June 29, 2019 18:41
@musm musm changed the base branch from julia-uv2-1.29.1 to julia-uv2-1.24.0 June 29, 2019 18:42
@musm musm changed the base branch from julia-uv2-1.24.0 to julia-uv2-1.29.1 June 29, 2019 18:45
@musm musm changed the base branch from julia-uv2-1.29.1 to julia-uv2-1.24.0 June 29, 2019 18:47
@musm musm closed this Jun 29, 2019
@StefanKarpinski
Copy link
Member

@musm, can you please leave PRs like this open even if they don't get merged right away? Sometimes it takes a while to get things reviewed and merged.

@musm
Copy link
Author

musm commented Jul 1, 2019

@StefanKarpinski I closed this since it's stale. The main reason is this branch needs to be updated to the latest libuv branch we are using julia-uv2-1.29.1, which is a trivial change, but it requires a new PR.

@StefanKarpinski
Copy link
Member

It's also possible to rebase the existing PR; keeping it open acts as a way of keeping track of the PR since closed PRs are generally never looked at again, whereas open PRs will be looked at.

@musm
Copy link
Author

musm commented Sep 23, 2019

Can't we just agree that the problem is not this PR, but the broken repl shell mode in julia?

I would be in favor of merging this PR. And either trying to fix the broken repl shell mode rather than holding up this useful PR, that is much more practically relevant.

The examples that break the repl shell mode are quite esoteric as evidence by what one has to do to trigger it.

@musm
Copy link
Author

musm commented Sep 23, 2019

Neither my hacking nor @davidanthoff have been able to successfully resolve the shell mode bug. And seeing that no one else has tried to resolve the issue, I think it should be appropriate to merge this PR in some form.

@musm
Copy link
Author

musm commented Sep 26, 2019

I would love some feedback, before trying to rebase this based on my previous comments.

@musm
Copy link
Author

musm commented Jul 9, 2020

Superseded by #11 which is opened against the latest julia libuv fork

(In other words, this is stale since it's opened against an old fork)

@musm musm closed this Jul 9, 2020
@musm musm deleted the julia-uv2-1.24.0 branch September 4, 2020 04:43
@musm musm restored the julia-uv2-1.24.0 branch September 4, 2020 04:43
@musm musm deleted the julia-uv2-1.24.0 branch September 4, 2020 04:43
@musm musm restored the julia-uv2-1.24.0 branch September 4, 2020 04:43
@musm musm deleted the julia-uv2-1.24.0 branch September 4, 2020 04:45
vtjnash pushed a commit that referenced this pull request Jul 20, 2021
ERROR: LeakSanitizer: detected memory leaks

```
Direct leak of 432 byte(s) in 9 object(s) allocated from:
    #0 0x1062eedc2 in __sanitizer_mz_calloc+0x92 (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x46dc2)
    #1 0x7fff20171eb6 in _malloc_zone_calloc+0x3a (libsystem_malloc.dylib:x86_64+0x1beb6)
    #2 0x7fff203ac180 in _CFRuntimeCreateInstance+0x124 (CoreFoundation:x86_64h+0x4180)
    #3 0x7fff203ab906 in __CFStringCreateImmutableFunnel3+0x84d (CoreFoundation:x86_64h+0x3906)
    #4 0x7fff203ab0a1 in CFStringCreateWithCString+0x48 (CoreFoundation:x86_64h+0x30a1)
    #5 0x1056f63e1 in uv__get_cpu_speed darwin.c:267
    #6 0x1056f491e in uv_cpu_info darwin.c:338
```

PR-URL: libuv#3098
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
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.

5 participants