classification
Title: _pyio module broken on Cygwin / setmode not usable
Type: crash Stage: patch review
Components: IO, Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: erik.bray, masamoto, zach.ware
Priority: normal Keywords: patch

Created on 2016-10-17 10:58 by erik.bray, last changed 2019-06-12 12:18 by erik.bray.

Files
File name Uploaded Description Edit
cygwin-pyio-setmode.patch masamoto, 2016-10-20 22:03 included other parts review
Pull Requests
URL Status Linked Edit
PR 14013 open erik.bray, 2019-06-12 12:18
Messages (5)
msg278802 - (view) Author: Erik Bray (erik.bray) * (Python triager) Date: 2016-10-17 10:58
Since the patch to #24881 was merged the _pyio module has been non-importable on Cygwin, due to an attempt to import from the msvcrt module.

However, the msvcrt module is not built on Cygwin (Cygwin does not use MSVCRT).  Cygwin's libc does, however, have _setmode.

The problem this is trying to solve is to call _setmode (https://msdn.microsoft.com/en-us/library/tw4k6df8.aspx) on file descriptors to ensure that they are O_BINARY instead of O_TEXT (a distinction that needs to made on Windows).

Fortunately, on Cygwin, it is fairly rare that a file descriptor will have mode O_TEXT, but it it is possible.  See https://cygwin.com/faq/faq.html#faq.programming.msvcrt-and-cygwin

This could be tricky to solve though.  Removing setmode() call entirely works for me as far as the test suite is concerned.  But it leaves _pyio slightly incongruous with the C implementation in this one small aspect, and it *is* a bug.

I would propose for Python 3.7 adding an os.setmode() function.  On Windows this could be simply an alias for msvcrt.setmode() (or vice-versa).  But because setmode() is available also in Cygwin (and technically some other platforms too, though none that are currently supported by Python) it would be a good candidate for inclusion in the os module I think (for those platforms that have it).
msg279086 - (view) Author: Masayuki Yamamoto (masamoto) * Date: 2016-10-20 22:03
I agree to use setmode() at _pyio module for Cygwin. However, I have two reasons that I disagree to the implementation of os.setmode().
First, FreeBSD has another setmode() that operate file permission bits [1]. Therefore the implementation of os.setmode() will be confused to those users.
Second, msvcrt.setmode() isn't used almost in standard library, the function is just used by _pyio module and subprocess test. Thus I think there aren't much need that implements setmode() to os module.

Easy way for a solution of import error on Cygwin, setmode() is implemented on _pyio using ctypes. then Cygwin CPython forget setmode().
Otherwise it's hard way, For avoiding confliction to other platforms, I'd propose to create the cygwin module. I thought a solution implementing imitation msvcrt module, but it is unreasonable because Cygwin doesn't have other Windows API.

I tried written the patch of easy way that implements setmode() into _pyio. This patch is included parts to succeed import _pyio.

[1] https://www.freebsd.org/cgi/man.cgi?query=setmode&sektion=3
msg280203 - (view) Author: Erik Bray (erik.bray) * (Python triager) Date: 2016-11-07 15:25
Hi Masayuki, thanks for the response (I thought I replied to this earlier but maybe I only imagined it--I've been on vacation).

I agree with just about everything you write here.

I'm aware of the FreeBSD setmode(), but I figured os.setmode() could do different things on different platforms as applicable.  But that would also be very confusing.

Your patch using ctypes looks fine to me--I considered doing the same, but what I'm not sure is if it's kosher to use ctypes here, given that it's techically an optional module.  Since _pyio is, as I understand it, mainly used for testing it's probably fine?  But I think we'll need some core maintainers' comments here...

Implementing a cygwin module would be really nice, actually, even if it isn't included in the stdlib.  There are a few other cygwin-specific APIs that it is useful to have wrappers around: https://cygwin.com/cygwin-api/cygwin-functions.html  But that would be outside the scope of fixing this issue.
msg308174 - (view) Author: Masayuki Yamamoto (masamoto) * Date: 2017-12-13 00:07
FYI, cygwin-pyio-setmode.patch includes two extra parts for running on Cygwin.  First, fix import error for ctypes (unopened issue). Second, fix building _ctypes module (#4032 - PR 4153).
msg308630 - (view) Author: Erik Bray (erik.bray) * (Python triager) Date: 2017-12-19 11:56
Right, the current patch is really combining several issues.  I'll make  a PR from it that just fixes the _pyio issue.  I'll also open an issue for fixing the ctypes bug (this is a patch I've had in my cygwin branch for ages but just never got around to making a report for...)
History
Date User Action Args
2019-06-12 12:18:56erik.braysetpull_requests: + pull_request13877
2018-03-23 20:05:53berker.peksaglinkissue32287 superseder
2017-12-21 19:19:17zach.waresetnosy: + zach.ware
stage: patch review

versions: - Python 3.5
2017-12-19 11:56:31erik.braysetmessages: + msg308630
2017-12-13 00:07:13masamotosetmessages: + msg308174
2016-11-07 15:25:03erik.braysetmessages: + msg280203
2016-10-20 22:03:26masamotosetfiles: + cygwin-pyio-setmode.patch

components: + Library (Lib)
versions: + Python 3.5, Python 3.6, Python 3.7
keywords: + patch
nosy: + masamoto

messages: + msg279086
2016-10-17 10:58:12erik.braycreate