Skip to content

[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

Merged
merged 2 commits into from
Feb 26, 2018

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Oct 27, 2017

(cherry picked from commit 63ae044)

https://bugs.python.org/issue31878

@serhiy-storchaka serhiy-storchaka changed the title [3.6] Fix _socket module compilation on Cygwin. (GH-4137) [3.6] bpo-31878: Fix _socket module compilation on Cygwin. (GH-4137) Oct 27, 2017
@miss-islington
Copy link
Contributor Author

@embray and @serhiy-storchaka: Backport status check is done, and it's a success ✅ .

Copy link
Member

@vstinner vstinner left a 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.

@bedevere-bot
Copy link

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.

@serhiy-storchaka
Copy link
Member

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.

@embray
Copy link
Contributor

embray commented Oct 27, 2017

@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!

@vstinner
Copy link
Member

vstinner commented Nov 6, 2017

"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

@serhiy-storchaka
Copy link
Member

This is a trivial change. And it doesn't add a new feature, it just fixes a regression introduced by bpo-13777.

@vstinner
Copy link
Member

vstinner commented Nov 6, 2017

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?

@ned-deily
Copy link
Member

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.

@embray
Copy link
Contributor

embray commented Nov 6, 2017

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).

@embray
Copy link
Contributor

embray commented Nov 6, 2017

@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.

@serhiy-storchaka
Copy link
Member

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.

@serhiy-storchaka serhiy-storchaka merged commit e650fd3 into python:3.6 Feb 26, 2018
@miss-islington
Copy link
Contributor Author

Thanks, @serhiy-storchaka!

@miss-islington miss-islington deleted the backport-63ae044-3.6 branch February 26, 2018 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants