Implement Python 2.7 support

[alekstorm]

Thanks for the comprehensive test suite; it made this port a snap. The only externally-facing change I had to make was removing the magic * argument in HTTP20Connection.__init__() - if you like, I can restore it on Python 3 with trickery a la SocketServerThread.

Barring the introduction of pyOpenSSL, supporting NPN is impossible on Python 2.7, so for now, the client assumes the server speaks HTTP/2.0 (referred to in the spec as "with prior knowledge").

An automatic migration tool like 2to3 was unneeded, and I think it's best to keep it that way.

[alekstorm]

It appears that although the tests all passed, Travis didn't like the missing code coverage. However, I'm not sure how to achieve 100% coverage, since (minimal) pieces of logic are gated on whether the current Python is 2 or 3. How are problems like this normally handled?

[Lukasa]

@alekstorm Thanks so much for this: I didn't actually expect you to do the work yourself! I haven't looked at the code yet so expect some review comments, but I'll answer your top level questions/comments now.

Firstly, I'm not that attached to the keyword-only argument stuff for Python 3. I liked it because it cleaned the API up, but not so much that it's worth bending over backward to keep.

I'm happy to support 2.7 without NPN for the moment, but I consider it to be a sad state of affairs. In particular, I suspect it hurts our interop prospects: given that HTTP/2 is usually served over the same port as HTTP/HTTPS, I suspect that without NPN/ALPN most servers won't actually talk to us. This means that we really need #28 to get a positive Python 2 story here. Given the existence of this pull request, I'm going to upgrade #28 from "Wouldn't It be Nice If" to "Long-term Goal".

I'm glad you didn't use 2to3, because I'd probably have rejected the pull request if you did! =D

Yes, I mandate 100% code coverage in this project, so we're going to need to fix that. I won't be able to give concrete advice about how to improve the coverage until I've reviewed the code, so I'll get back to you on it.

Finally, I'm glad the test suite was helpful! I certainly spent enough time on it. =P

Once again, thanks for this, it's a huge help! 🍰

[Lukasa]

For things like this we should steal the requests approach, which is to have a file called compat that pastes over the differences. That way we can avoid having this horrible conditional import logic at the top of all our files.

[Lukasa]

Alright, code review inline. One of these notes requires you to restructure the project which should hopefully make it much easier for us to fixup the code coverage problems, so I'll hold off on advice there until you've acted on the review comments.

As always, if you disagree with a review comment let me know. =) Otherwise, let me know when you're done and I'll take another look.

[Lukasa]

Also, don't forget to add yourself to AUTHORS.rst!

[alekstorm]

I'm glad you're open to supporting Py2.7 for now, even without NPN/ALPN support. I agree it's far from ideal; I hope to rectify the situation in an upcoming PR.

[alekstorm]

Er, I'm flattered, but there is no AUTHORS.rst. Would you like me to create it? Although it's your project; I think it's best if you decide the formatting.

[Lukasa]

Heh, I apologise, I meant CONTRIBUTORS.rst.

[alekstorm]

Aaand I missed that, somehow. Thanks.

On Wed, Apr 2, 2014 at 2:28 PM, Cory Benfield notifications@git.luolix.topwrote:

Heh, I apologise, I meant CONTRIBUTORS.rst.

Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-39386118
.

[alekstorm]

Okay, done. Most compatibility logic was moved into the new compat.py, and the SSL verify_mode monkey-patching was fixed. Did I miss anything?

[alekstorm]

Just finished a compatibility layer that exposes pyOpenSSL as an API equivalent to the standard Python 3.x ssl module, obviating the need for separate Python 2.7 logic in tls.py and server.py. Should I push the commit to this branch or #34's? It's not strictly required for Python 2.7 support, but does handily solve the code coverage problem - py.test can be instructed to ignore both compat.py and ssl_compat.py.

[Lukasa]

This looks great, I don't see any further changes required here. I don't understand why Travis reported such bad code coverage, so I'll have to dig into that myself.

As for a compatibility layer, it presumably requires a version of PyOpenSSL newer than the current release, considering that NPN support isn't in the current one? If that's the case, it'd be better as a new Pull Request. =)

[alekstorm]

Sure, that's no problem (although it's worth noting that the compatibility layer is only used on Py2.7).

[Lukasa]

We should avoid the compatibility layer being used on only one platform: we should use it consistently. Fewer bugs. =)

[alekstorm]

Hm, I think you're right; in any case, we'll end up using it very often on Py<=3.4, since ALPN support will only show up in 3.5 at the earliest (I think it's best to avoid making pyOpenSSL a hard dependency).

I predict a complex interaction of Python version, OpenSSL version, and presence of pyOpenSSL will determine the available feature set for a given installation of hyper. As long as it's thoroughly documented, I don't think this is a problem.

[Lukasa]

Mm, you've brought up an interesting point. A fun fact is that Python 3.X bundles its own copy of OpenSSL on OS X because the system copy is woefully out of date. This means that PyOpenSSL will actually regress NPN support on OS X unless you install a newer version of OS X and point PyOpenSSL at it, which is a pain in the ass.

Let's make PyOpenSSL a dependency based on software version, and currently let's limit it to 2.7 to avoid breaking anything for anyone for the moment.

[alekstorm]

Excellent plan - let's get the code out there, then progressively enable it for more environments once we understand exactly what's going on.

P.S. I did not know that Py3 bundles its own OpenSSL; thanks for the heads up. So now there's the system OpenSSL, the homebrew version, and the Py3 bundled version, all having a party on my box at once.

[Lukasa]

Yeah, it's a total mess. For some background, OpenSSL basically hasn't been updated in years, and the core dev team ended up taking the same approach as they do on Windows Python releases (which also bundle OpenSSL).

The various OpenSSL compatibility issues are a nightmare, and PyOpenSSL doesn't really improve them. I'm not looking forward to telling OS X users that they need to set environment variables before installing hyper. =( However, this is the world in which we live.

[Lukasa]

What the hell is going on with code coverage? Running the tests on my Windows box gets the exact same failure, which is insane because it's pointing at lines that can't possibly be being missed. I wonder if there was an update to any of those packages recently.

[Lukasa]

Well the last successful Travis build has the same versions of all dependencies as this one, so I'm thoroughly confused.

[Lukasa]

Oh! You're importing from the project in conftest.py, which means the lines that are executed on import don't get included. Let's just run coverage separately from py.test, that should fix it. Gimme a sec and I'll push up a fix for you to rebase on to.

[Lukasa]

Actually, that won't work with py.test xdist, so I have a new plan. All you import from hyper in conftest.py is open, and that's not used anywhere in the project itself. So let's just do that definition in conftest.py without importing from hyper. Are you happy to make that change?

[alekstorm]

Sure, made the change, and added compat.py to the list of omitted files in .coveragerc. Spent a good 45 minutes trying to convince py.test and coverage to generate a combined report for both Py2.7 and 3.3 before giving up - what happens next depends on your personal preference. We could:

1. Spend (possibly a lot) more time wrangling a combined coverage report out of the tooling
2. Add pragma: no cover to the two definitions of wrap_socket() in tls.py
3. Merge the PR as-is and damn the consequences

I'm in favor of 2) or 3), and don't have a strong opinion on which one; they both introduce a problem that will be solved by the merging of #34, which includes the SSL compatibility layer (and hence, requires only a single definition of wrap_socket()).

[Lukasa]

I'm in favour of 2), so let's go ahead and do that. =)

[alekstorm]

Hey, lookit that, a green build.

[Lukasa]

Brilliant, thanks! 🍰 🍪 ☕ 🍺

[alekstorm]

No problem, thanks for all your help.