bpo-33709: test_ntpath and test_posixpath fail in Windows with ACP!=1252 by native-api · Pull Request #7278 · python/cpython (original) (raw)

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

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

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).

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.

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.

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.