_pyio checks that `os.name == 'win32'` instead of 'nt'

[CosimoLupo]

BPO	24881
Nosy	@pfmoore, @vstinner, @tjguk, @4kir4, @embray, @zware, @serhiy-storchaka, @eryksun, @zooba
Files	pyio_setmode.diff: call setmode(fd, O_BINARY) if sys.platform in {'win32', 'cygwin'} like in C implementation

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2015-08-28.19:22:18.911>
created_at = <Date 2015-08-17.14:11:07.420>
labels = ['type-bug', 'expert-IO', 'OS-windows']
title = "_pyio checks that `os.name == 'win32'` instead of 'nt'"
updated_at = <Date 2016-10-17.10:37:42.399>
user = 'https://bugs.python.org/CosimoLupo'

bugs.python.org fields:

activity = <Date 2016-10-17.10:37:42.399>
actor = 'vstinner'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2015-08-28.19:22:18.911>
closer = 'serhiy.storchaka'
components = ['Windows', 'IO']
creation = <Date 2015-08-17.14:11:07.420>
creator = 'Cosimo Lupo'
dependencies = []
files = ['40222']
hgrepos = []
issue_num = 24881
keywords = ['patch']
message_count = 7.0
messages = ['248728', '248978', '248986', '249290', '249291', '278800', '278801']
nosy_count = 11.0
nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'akira', 'python-dev', 'erik.bray', 'zach.ware', 'serhiy.storchaka', 'eryksun', 'steve.dower', 'Cosimo Lupo']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24881'
versions = ['Python 3.5', 'Python 3.6']

[CosimoLupo]

the _pyio module at line 16 tries to check whether it is running on Windows platform, by doing:

if os.name == 'win32':
 from msvcrt import setmode as _setmode
else:
 _setmode = None

However, the string returned by os.name is 'nt' and not 'win32' (the latter is returned by sys.platform). Therefore, the value is always False and the setmode function from mscvrt module is never imported.

Thank you.
Cheers,

Cosimo

[4kir4]

To make _pyio correspond to the C version I've added

sys.platform in {'win32', 'cygwin'}

condition. See the attached pyio_setmode.diff

It is not clear why the absence of _setmode(fd, os.O_BINARY) is not detected by tests.

(a) a corresponding test should be added
(b) OR _setmode() call should be removed from both Python and C io versions

[eryksun]

It is not clear why the absence of _setmode(fd, os.O_BINARY)
is not detected by tests.

It's only a problem when an existing text-mode file descriptor is passed in. For example, in text mode the CRT handles b'\x1a' as an EOF marker:

 Python 3.5.0b4 (v3.5.0b4:c0d641054635, Jul 26 2015, 07:11:12)
 [MSC v.1900 64 bit (AMD64)] on win32
 Type "help", "copyright", "credits" or "license" for more information.
 >>> import os, _pyio
 >>> _ = open('testfile', 'wb').write(b'abc\x1adef')

 >>> fd = os.open('testfile', os.O_RDONLY|os.O_TEXT)
 >>> f = _pyio.open(fd, 'rb')
 >>> f.read()
 b'abc'
 >>> f.close()

 >>> f = _pyio.open('testfile', 'rb')
 >>> f.read()
 b'abc\x1adef'

The setmode call ensures a file descriptor is in the expected binary mode:

 >>> import msvcrt
 >>> fd = os.open('testfile', os.O_RDONLY|os.O_TEXT)
 >>> old_mode = msvcrt.setmode(fd, os.O_BINARY)
 >>> old_mode == os.O_TEXT
 True
 >>> f = _pyio.open(fd, 'rb')
 >>> f.read()
 b'abc\x1adef'

[python-dev]

New changeset 687da8760a58 by Serhiy Storchaka in branch '3.5':
Issue bpo-24881: Fixed setting binary mode in Python implementation of FileIO
https://hg.python.org/cpython/rev/687da8760a58

New changeset 2dd9294f679d by Serhiy Storchaka in branch 'default':
Issue bpo-24881: Fixed setting binary mode in Python implementation of FileIO
https://hg.python.org/cpython/rev/2dd9294f679d

[serhiy-storchaka]

Good catch. Thank you for your report Cosimo. Thank you for your patch Akira.

[embray]

FWIW this patch broke the _pyio module on Cygwin, as the msvcrt module is not built on Cygwin. AFAICT this is only a problem for Python built with MSVCRT, which Cygwin does not use. When test case works as expected on Cygwin without this.

[vstinner]

"FWIW this patch broke the _pyio module on Cygwin,"

Please open a new issue, this one is closed.

[vstinner]

FWIW this patch broke the _pyio module on Cygwin, as the msvcrt module is not built on Cygwin.

I wrote PR gh-149899 to fix the regression.