Skip to content

bpo-33709: test_ntpath and test_posixpath fail in Windows with ACP!=1252 #7278

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

Merged
merged 3 commits into from
Dec 6, 2018

Conversation

native-api
Copy link
Contributor

@native-api native-api commented May 31, 2018

@native-api native-api force-pushed the test_path_unicode_mbcs branch from d19fdae to 750291e Compare May 31, 2018 09:15
@native-api
Copy link
Contributor Author

#7279 is backport to 2.7 (bedevere didn't detect it 'cuz it was added earlier)

@native-api
Copy link
Contributor Author

Travis failure caused by https://bugs.python.org/issue33532

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't have the bandwidth to test properly your change on Windows right now, but I don't think that it's correct.

Python uses UTF-8 to encode paths on Windows since Python 3.6: https://vstinner.github.io/python36-utf8-windows.html It should not use the ANSI code page for filenames anymore (except if you explicitly opt-in for the ANSI code page).

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@native-api native-api force-pushed the test_path_unicode_mbcs branch from f4fb2d8 to 9c9db38 Compare May 31, 2018 22:24
@native-api
Copy link
Contributor Author

native-api commented May 31, 2018

I have made the requested changes; please review again.

Yes, it doesn't cause test failures in stock configuration for 3.6+.
3.6+ can still be configured to use mbcs via environment though (https://docs.python.org/3/library/sys.html#sys._enablelegacywindowsfsencoding), and I checked that it does cause failures in such a case:

> set PYTHONLEGACYWINDOWSFSENCODING=1
> python.bat -m test -v test_ntpath
<...>
======================================================================
FAIL: test_expandvars_nonascii (test.test_ntpath.NtCommonTest)
----------------------------------------------------------------------
<...>
AssertionError: b'${?}bar' != b'ham?bar'

Since test code is not supposed to check if FS encoding is the default or anything before using this logic, it should work correctly in any supported configuration.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@vstinner, @serhiy-storchaka: please review the changes made to this pull request.

@bedevere-bot
Copy link

GH-7279 is a backport of this pull request to the 2.7 branch.

@zooba
Copy link
Member

zooba commented Jun 4, 2018

I agree this should be fixed, but I'd also like to see a comment in 3.x clearly indicating that this only matters if you're using the legacy encoding mode.

@native-api
Copy link
Contributor Author

@zooba Already did that in 9c9db38 .

@serhiy-storchaka
Copy link
Member

It looks to me that all comments were addressed. I'm going to merge this PR.

@vstinner vstinner dismissed their stale review December 6, 2018 00:25

PR changed since I reviewed it

@serhiy-storchaka serhiy-storchaka merged commit 8752dfb into python:master Dec 6, 2018
@miss-islington
Copy link
Contributor

Thanks @native-api for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-10981 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 6, 2018
@bedevere-bot
Copy link

GH-10982 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 6, 2018
miss-islington added a commit that referenced this pull request Dec 6, 2018
miss-islington added a commit that referenced this pull request Dec 6, 2018
@native-api native-api deleted the test_path_unicode_mbcs branch December 6, 2018 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip issue skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants