classification
Title: _pyio checks that `os.name == 'win32'` instead of 'nt'
Type: behavior Stage: resolved
Components: IO, Windows Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Cosimo Lupo, akira, erik.bray, eryksun, paul.moore, python-dev, serhiy.storchaka, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2015-08-17 14:11 by Cosimo Lupo, last changed 2016-10-17 10:37 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
pyio_setmode.diff akira, 2015-08-21 20:18 call setmode(fd, O_BINARY) if sys.platform in {'win32', 'cygwin'} like in C implementation review
Messages (7)
msg248728 - (view) Author: Cosimo Lupo (Cosimo Lupo) Date: 2015-08-17 14:11
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
msg248978 - (view) Author: Akira Li (akira) * Date: 2015-08-21 20:18
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
msg248986 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2015-08-22 03:01
> 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'
msg249290 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-08-28 19:20
New changeset 687da8760a58 by Serhiy Storchaka in branch '3.5':
Issue #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 #24881: Fixed setting binary mode in Python implementation of FileIO
https://hg.python.org/cpython/rev/2dd9294f679d
msg249291 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-08-28 19:22
Good catch. Thank you for your report Cosimo. Thank you for your patch Akira.
msg278800 - (view) Author: Erik Bray (erik.bray) * (Python triager) Date: 2016-10-17 10:26
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.
msg278801 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-10-17 10:37
"FWIW this patch broke the _pyio module on Cygwin,"

Please open a new issue, this one is closed.
History
Date User Action Args
2016-10-17 10:37:42vstinnersetmessages: + msg278801
2016-10-17 10:26:17erik.braysetnosy: + erik.bray
messages: + msg278800
2015-08-28 19:22:18serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg249291

stage: patch review -> resolved
2015-08-28 19:20:53python-devsetnosy: + python-dev
messages: + msg249290
2015-08-28 19:03:15serhiy.storchakasetassignee: serhiy.storchaka
stage: patch review
2015-08-22 03:01:07eryksunsetnosy: + eryksun
messages: + msg248986
2015-08-21 20:55:51serhiy.storchakasetnosy: + vstinner, serhiy.storchaka

versions: - Python 3.4
2015-08-21 20:18:40akirasetfiles: + pyio_setmode.diff

nosy: + akira
messages: + msg248978

keywords: + patch
2015-08-17 14:43:18zach.waresetnosy: + paul.moore, tim.golden, zach.ware, steve.dower

components: + Windows
versions: + Python 3.4, Python 3.6
2015-08-17 14:11:07Cosimo Lupocreate