Add Unicode support for process spawning on Windows #14075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice work, this looks great! |
|
Updated. |
|
Looks great, thanks! Could you squash the last two commits up into the first ones? It also looks like this needs a rebase. |
|
Thanks :) Done and ran the tests. |
Changed libnative to use CreateProcessW instead of CreateProcessA. In addition, the lpEnvironment parameter now uses Unicode. Added a helper function os::win32::as_mut_utf16_p, which does basically the same thing as os::win32::as_utf16_p except the pointer is mutable.
Previously, make_command_line iterates over each u8 in the string and then appends them as chars, so any non-ASCII string will get horribly mangled by this function. This fix should allow Unicode arguments to work correctly in native::io::process::spawn.
Changed libstd to use Get/FreeEnvironmentStringsW instead of Get/FreeEnvironmentStringsA to support Unicode environment variables.
Added a run-pass test to ensure that processes can be correctly spawned using non-ASCII arguments, working directory, and environment variables. It also tests Unicode support of os::env_as_bytes. An additional assertion was added to the test for make_command_line to verify it handles Unicode correctly.
|
Fixed the merge conflict. |
|
Ooh, happy to see this landing! Thanks for the heads-up. |
bors
added a commit
that referenced
this pull request
May 13, 2014
- Use Unicode-aware versions of `CreateProcess` (Fixes #13815) and `Get/FreeEnvironmentStrings`. - Includes a helper function `os::win32::as_mut_utf16_p`, which does the same thing as `os::win32::as_utf16_p` except the pointer is mutable. - Fixed `make_command_line` to handle Unicode correctly. - Tests for the above.
| Some(dir) => dir.with_c_str(|buf| cb(buf)), | ||
| Some(dir) => match dir.as_str() { | ||
| Some(dir_str) => os::win32::as_utf16_p(dir_str, cb), | ||
| None => cb(ptr::null()) |
There was a problem hiding this comment.
This silently removes the working directory configuration if it's not in utf8 format (i.e., when dir.as_str() returns None). It should fail loudly, using expect.
Don't worry about changing this, though -- I'm currently rebasing another patch around this one, for dealing with non-unicode filesystems/arguments on other platforms, and I'll address the problem there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CreateProcess(Fixes libnative uses CreateProcessA on windows #13815) andGet/FreeEnvironmentStrings.os::win32::as_mut_utf16_p, which does the same thing asos::win32::as_utf16_pexcept the pointer is mutable.make_command_lineto handle Unicode correctly.