-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
[3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137) #4145
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
[3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137) #4145
Conversation
(cherry picked from commit 63ae044)
@embray and @serhiy-storchaka: Backport status check is done, and it's a success ✅ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait. For me, Cygwin means supporting a new platform. It was never supported officially. I'm against backporting such change.
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 |
We have no Cygwin buildbots, but the code for supporting Cygwin already is in the sources. Cygwin even is mentioned in the |
@serhiy-storchaka @Haypo Indeed, the purpose of the fixes I'm submitting now are in the support of getting a reliable Cygwin buildbot running (and possibly Appveyor CI as well). I have a ton of other patches for fixing various test failures, but the patches I'm submitting right now are required to: a) Build Python at all Having such fixes backported to 3.6 where appropriate will also be a big help! |
"I have a ton of other patches for fixing various test failures, but the patches I'm submitting right now are required to: (...)" I dislike merging "ton of patches" in a stable Python version, to support a platform which is not officially supported. Please work only in the master branch, I will try to help you to get patches merged. Once Cygwin is fully supported in master, we can start discussing to backport or not such changes. cc @ned-deily the Python 3.6 release manager |
This is a trivial change. And it doesn't add a new feature, it just fixes a regression introduced by bpo-13777. |
A regression compared to which Python version? This change was made 5 years ago. Regression compared to Python 2.7? Is it possible to build Python 2.7 without any change with MinGW? |
Obviously, Cygwin support in Python has been sketchy at best for many years. While it would be nice to have it "officially" supported as other major platforms are, it is true that Cygwin was not truly supported when 3.6 was released. So, I agree with @Haypo that we should focus on getting Cygwin working with 3.7 as if it were a feature (perhaps even calling it out as such with a PEP, if necessary) and then decide how much of that work, if any, we should backport to 3.6. In that light, since the work is already done and trivial, I won't object to merging this PR for 3.6 but I think we should hold off on further 3.6 changes and stick to master. |
If it helps, I can write up a PEP and/or update to PEP-11 clarifying Cygwin support. I've been meaning to offer to do so for a while but I wanted to get a Buildbot up and running first in order to demonstrate a degree of commitment. But on the other hand, having a well-functioning Buildbot means having a number of fixes in place (preferably not on a personal branch) so it's a chicken-egg thing. FWIW I also have an AppVeyor build on Cygwin running at: https://ci.appveyor.com/project/embray/cpython (currently down to 26 test failures, though there are a few extension modules that aren't being built yet). |
@ned-deily and @Haypo have a point w.r.t. this PR. In fact without #1362 (PEP-539) backported to 3.6, any other Cygwin-specific fixes in 3.6.x are next to useless anyways (Python 3.6 can be built on Cygwin So while I don't see the harm in such a backport, so long as there's objection it's fine to omit for now. |
The change is trivial and I already changed this line for fixing support on NetBSD. I think it will not harm if backport this change to 3.6. |
Thanks, @serhiy-storchaka! |
(cherry picked from commit 63ae044)
https://bugs.python.org/issue31878