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

curl: revert #3011 and write UTF-8 in current console code page. #3212

Closed
wants to merge 3 commits into from

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Nov 1, 2018

Current implementation for output UTF-8 on Windows console doesn't restore font. Windows console set font to suitable possibly. And restoring code page also set suitable font. So font is not restored correctly. At least, this implementation doesn't work on Japanese locale on Windows. To write UTF-8 without changing console code page, it should use Wide String APIs. If output stream is not a TTY, do original behavior. Thanks.

image

You notice some borders in this screenshot are not displayed in correct location. This is problem of fonts not curl. If you set Raster Font for the console, it probably works well.

image

Fixes #3211

@vszakats
Copy link
Member

vszakats commented Nov 1, 2018

Based on past experience changing the codepage may even result in locked up consoles on certain systems. This was a case I could never actually reproduce locally but it started to happen for multiple users after deploying a similar solution. Another related issue was when execution stopped abnormally (e.g. via Ctrl+C) skipping the CP restoration code. But, even with a Ctrl+C handler, the locking/garbled console issues weren't fully resolved. The solution back then was exactly the same as above, using the WriteConsole() API to create the output without touching the selected CP.

@bagder bagder added cmdline tool Windows Windows-specific labels Nov 1, 2018
@bagder
Copy link
Member

bagder commented Nov 1, 2018

There's just some code style nits that travis points out:

./tool_cb_wrt.c:160:5: warning: if with space (SPACEBEFOREPAREN)
   if (is_tty) {
     ^
./tool_cb_wrt.c:168:7: warning: if with space (SPACEBEFOREPAREN)
     if (!wc_buf)
       ^
./tool_cb_wrt.c:174:7: warning: if with space (SPACEBEFOREPAREN)
     if (!WriteConsoleW(
       ^

@bagder
Copy link
Member

bagder commented Nov 2, 2018

Interestingly enough, this causes test 1426 to fail in appveyor builds. Test 1426 does this:

curl http://url/ --output -

and the body of the response contains 5 bytes. A binary zero, and then 1234. It fails with this error message

curl: (23) Failed writing body (0 != 5)

@mattn
Copy link
Contributor Author

mattn commented Nov 2, 2018

image

seems to be fail on Debug tests. I'll look into it in few days.

@mattn
Copy link
Contributor Author

mattn commented Nov 2, 2018

Ah, is_tty become TRUE when DEBUGBUILD.

@mattn
Copy link
Contributor Author

mattn commented Nov 2, 2018

Could you please teach me how to run test just test1426?

@MarcelRaad
Copy link
Member

MarcelRaad commented Nov 2, 2018

@mattn

Could you please teach me how to run test just test1426?

cd tests && ./runtests.pl 1426

See also https://curl.haxx.se/dev/runtests.html.

@mattn
Copy link
Contributor Author

mattn commented Nov 3, 2018

@MarcelRaad Thank you.

It seems tests are passed.

@bagder
Copy link
Member

bagder commented Nov 5, 2018

Thanks!

@bagder bagder closed this in 5bfaa86 Nov 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cmdline tool Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

4 participants