Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-133374: Fix test_python_legacy_windows_stdio in test_cmd_line#133375
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
LGTM
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.
I suspect there's a good chance it's not getting real console handles in CI systems either... maybe we should also printtype(sys.stdout)
(or possibly we have to dig into.buffer.raw
)?
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
aisk commentedMay 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Tested it onmy own repo with: importsysprint(sys.stdin.encoding,sys.stdout.encoding)print(sys.stdin.buffer.raw,sys.stdout.buffer.raw) And the result is:
Instead of what I've got on my local machine: utf-8utf-8<_io._WindowsConsoleIOmode='rb'closefd=False><_io._WindowsConsoleIOmode='wb'closefd=False> So I think there is no real consoles on GHA, and we can't run this case on it. I guess we can just add |
After reading#114565, I'm not sure whether However, I found that it is not enabled or run in CI because the If we intend to enable the |
Or we can just skip the test partway through ( |
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 does not test the stderr encoding.
We should use other way to pass data from subprocess. For example, temporary file or shared file descriptor (if this is supported on Windows).
out = p.stderr.read() | ||
p.stderr.close() | ||
p.wait() | ||
self.assertNotIn(b'utf-8', out) |
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 might not be true. It's entirely possible to set the default encoding to UTF-8 in Windows itself, at which point the legacy stdio will also be UTF-8.
Checking the type of the stdin/stdout objects in the subprocess is probably the best way. Thereal purpose of this environment variable is to go back to usingFileIO
rather than_WindowsConsoleIO
, and the encoding is only a side-effect of that.
And if we just make the child processassert
, then we can check the exit code rather than reading the output.
Uh oh!
There was an error while loading.Please reload this page.
test_python_legacy_windows_stdio
intest_cmd_line
isn't actually working #133374