-
Notifications
You must be signed in to change notification settings - Fork 13.3k
UTF-16 console support for Windows #15051
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
Conversation
lpBuffer: LPCVOID, | ||
nNumberOfCharsToWrite: DWORD, | ||
lpNumberOfCharsWritten: LPDWORD, | ||
lpReserved: LPVOID) -> BOOL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to keep things out of liblibc
nowadays, could you add these to c_win32.rs
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(inside of libnative)
It would be nice to update libuv at the same time. If you open a PR against the |
fn tty_open(&mut self, fd: c_int, _readable: bool) | ||
-> IoResult<Box<rtio::RtioTTY + Send>> { | ||
use ERROR = libc::ERROR_INVALID_HANDLE; | ||
if unsafe { libc::isatty(fd) } != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a windows-specific implementation, it should probably use the windows-specific function GetConsoleMode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetConsoleMode
merely tells us the set of flags that we've set for the console, not whether it is or isn't a TTY. isatty
does the job fine at telling whether we're attached to a TTY or just redirected to a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it suffices for libuv, and I tend to not trust posix functions with file descriptors on windows, but if it's working well I guess it's working well.
No, setting the code page to UTF-8 does not work. I have already tested that option extensively in C++. The Windows console only supports narrow byte encodings and UTF-16. |
Ah, so output with UTF-8 does indeed work, but I've used both |
Even still, when using UTF-8, Windows will often act buggy. For some examples: It is probably safer to simply use UTF-16 because that is what Windows is built around. |
Added another commit to take into account comments by @alexcrichton |
} else { | ||
// If its not a tty, then it was redirected to a file | ||
// So just roll with it instead of erroring | ||
Ok(box file::FileDesc::new(fd, true) as Box<rtio::RtioTTY + Send>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not happen here. It's the responsibility of the caller to fall back to a file descriptor. This should have the same error path as the unix equivalent.
Oh dear, sorry for being so late to this! Feel free to ping a PR whenever you update this because sadly github doesn't send out notifications when that happens. |
The bors build failed due to the following error:
A fix for this is already upstream in libuv: joyent/libuv@6de622c I have created a pull request that updates rust's libuv: rust-lang-deprecated/libuv#3 Also, rebased and squashed my changes onto the latest rust master and verified that it works. Make sure you update the libuv repo before doing an r+ or the travis build will fail as it is now. |
Thanks again! |
This is the point where I break down and cry because the test that failed on the windows botbuild passes just fine for me, so I can't even replicate the error in an attempt to fix it. EDIT: And Travis doesn't feel like working either because building librustc takes more than 10 minutes. |
After doing a fresh clone of my fork and rebuilding, I still cannot reproduce the error on my end.
|
I updated the |
Sneaky! Nice catch! |
This implementation does have the minor issue of not handling things correctly when a codepoint is split across multiple writes or reads, but its better than not having unicode support at all. Adds a Windows specific struct `WindowsTTY` in `libnative` and make `tty_open` create that struct on Windows. Adds needed functions and constants to `c_win32.rs`. Also updates the libuv submodule. This fixes #15028 for both libnative and libgreen.
One potential reason for why that test is failing is because |
Adds a WindowsTTY for libnative that converts between UTF-8 and UTF-16. Signed-off-by: Peter Atashian <[email protected]>
This implementation does have the minor issue of not handling things correctly when a codepoint is split across multiple writes or reads, but its better than not having unicode support at all. Adds a Windows specific struct `WindowsTTY` in `libnative` and make `tty_open` create that struct on Windows. Adds needed functions and constants to `c_win32.rs`. Libuv still needs to be updated before #15028 can be closed.
This implementation does have the minor issue of not handling things correctly when a codepoint is split across multiple writes or reads, but its better than not having unicode support at all.
Adds a Windows specific struct
WindowsTTY
inlibnative
and maketty_open
create that struct on Windows. Adds needed functions and constants toc_win32.rs
.Libuv still needs to be updated before #15028 can be closed.