search

toland / patron

Ruby HTTP client based on libcurl
http://toland.github.com/patron/
MIT License
541 stars 73 forks source link

New libCURL options for HTTP Version and SSL Version #141

Closed saeohioalpha closed 7 years ago

saeohioalpha commented 7 years ago

Added support for ssl_version options "TLSv1_0", "TLSv1_1", "TLSv1_2", and "TLSv1_3" with preprocessor sections to allow compilation on systems with older versions of libCURL

Added new option http_version and options "None", "HTTPv1_0", "HTTPv1_1", "HTTPv2_0", "HTTPv2_TLS", and "HTTPv2_PRIOR" with preprocessor sections to allow compilation on systems with older versions of libCURL.

Added associated error/exceptions for http_version based on the errors for ssl_version.

Ubuntu 16.04 comes with libcurl v7.47.0 tested with that version and the latest 7.54.0

Tested with both versions "POST /users HTTP/2.0" 200 148 "-" "Patron/Ruby-0.8.0-libcurl/7.54.0 OpenSSL/1.1.0e zlib/1.2.8 nghttp2/1.23.0-DEV librtmp/2.3"

toland commented 7 years ago

This is a great-looking PR at first glance. I will be out of town for the holiday weekend, but I will take a closer look early next week.

julik commented 7 years ago

@saeohioalpha looks great! Really neat trick with the curl version comparison - I should use that for my other PR that's been in limbo for a while ;-)

Couple of driveby remarks if you don't mind. All the travis builds fail - apparently the Travis curl version is too old for both the HTTP2 and the TLS selection tests. These tests should skip explicitly with an error message IMO if running against a libcurl that doesn't support them. If we want to test features only available in libcurl that's newer than what Travis offers we should have a build matrix which manually downloads/builds libcurl with all the bells and whistles in addition to the libcurl Travis provides, build patron with that and runs the tests.

Additionally the indentation in the .c file has become inconsistent - we used 2 spaces everywhere, and some lines now have 4 spaces (or tabs, didn't check). It's true that Ruby C extensions do not have a strict formatting guideline attached but given that the existing .c file is already pretty big I think having consistent indentation in it has some merit, primarily for scanning through quickly. @toland thoughts?

toland commented 7 years ago

I agree with @julik on both points. We do need passing tests and I think we should keep the formatting consistent.

saeohioalpha commented 7 years ago

Take a look now and see if it matches up. I made the indentation consistent there, at least It looked like it to me!

julik commented 7 years ago

Thanks @saeohioalpha LGTM and merging ;-)