[3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137) by miss-islington · Pull Request #4145 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits2 Checks0 Files changed
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 }})
(cherry picked from commit 63ae044)
[3.6] Fix _socket module compilation on Cygwin. (GH-4137) [3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137)
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 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.
We have no Cygwin buildbots, but the code for supporting Cygwin already is in the sources. Cygwin even is mentioned in the README.rst build instruction. I don't see a hart in backporting this trivial change. This can help unofficial builds on Cygwin.
@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
b) Run the test suite from start to finish (not necessarily with every test passing; but there are some bugs that cause hangs/segfaults in the test suite that prevent it from finishing--fixes for that coming soon).
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.
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 --without-threads but who wants that?).
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.