| Message ID | patch-v2-5.8-2567b888c3d-20210910T105523Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | post-v2.33 "drop support for ancient curl" follow-up | expand |
On Fri, Sep 10, 2021 at 01:04:30PM +0200, Ævar Arnfjörð Bjarmason wrote: > In d73019feb44 (http: add support selecting http version, 2018-11-08) > a dependency was added on CURL_HTTP_VERSION_2, but this feature was > introduced in curl version 7.43.0, not 7.47.0, as the incorrect > version check led us to believe. > > As looking through the history of that commit on the mailing list will > reveal[1], the reason for this is that an earlier version of it > depended on CURL_HTTP_VERSION_2TLS, which was introduced in libcurl > 7.47.0. > > But the version that made it in in d73019feb44 had dropped the > dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version > check was not corrected. > > The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added in > 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in > 7.47.0. So we could support an even older version here, but let's just > correct the checked version. Thanks for expanding on the history here. I agree it probably doesn't matter much between the two versions, as they're both 6+ years old (and only about 6 months apart). If somebody has a case where it really matters, they can submit a patch. -Peff
On Fri, 10 Sep 2021, Jeff King wrote: >> The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added in >> 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in 7.47.0. So >> we could support an even older version here, but let's just correct the >> checked version. > > Thanks for expanding on the history here. I agree it probably doesn't matter > much between the two versions, as they're both 6+ years old (and only about > 6 months apart). If somebody has a case where it really matters, they can > submit a patch. Forgive me for digressing a bit here but wow, I *so* appreciate your digging into the details of the curl history and the symbols that were introduced when etc. I know of no other libcurl-using project with this eye and sense for historic details and as the lead maintainer of libcurl I learn a lot here. It also keeps me motivated to provide this documentation and work on keeping in accurate. Keep it up! <3
On Fri, Sep 10, 2021 at 05:20:24PM +0200, Daniel Stenberg wrote: > On Fri, 10 Sep 2021, Jeff King wrote: > > > > The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added > > > in 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in > > > 7.47.0. So we could support an even older version here, but let's > > > just correct the checked version. > > > > Thanks for expanding on the history here. I agree it probably doesn't > > matter much between the two versions, as they're both 6+ years old (and > > only about 6 months apart). If somebody has a case where it really > > matters, they can submit a patch. > > Forgive me for digressing a bit here but wow, I *so* appreciate your digging > into the details of the curl history and the symbols that were introduced > when etc. I know of no other libcurl-using project with this eye and sense > for historic details and as the lead maintainer of libcurl I learn a lot > here. It also keeps me motivated to provide this documentation and work on > keeping in accurate. This documentation is most definitely appreciated. As is your continued support and attention to our issues, not to mention just having libcurl in general. :) There are two related things that came up in this discussion that might be of interest to you: - it would be convenient if libcurl provided preprocessor macros indicating a feature was present. E.g., if we could say: #ifdef CURL_HAVE_FOO rather than checking that "FOO" showed up in version 7.60.0 or whatever. That has two advantages. One, it's just less work and harder to get wrong. But two, it could help for cases where distros backport features (i.e., the version-to-feature mapping is not always 100% correct). Obviously that can't help historical versions, but maybe something to think about as new features are added. - Ævar mentioned at the end of: https://lore.kernel.org/git/87tuiscwso.fsf@evledraar.gmail.com/ that the tags in the curl repo sometime seem to differ from the releases. -Peff
On Fri, Sep 10 2021, Daniel Stenberg wrote: > On Fri, 10 Sep 2021, Jeff King wrote: > >>> The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added >>> in 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in >>> 7.47.0. So we could support an even older version here, but let's >>> just correct the checked version. >> >> Thanks for expanding on the history here. I agree it probably >> doesn't matter much between the two versions, as they're both 6+ >> years old (and only about 6 months apart). If somebody has a case >> where it really matters, they can submit a patch. > > Forgive me for digressing a bit here but wow, I *so* appreciate your > digging into the details of the curl history and the symbols that were > introduced when etc. I know of no other libcurl-using project with > this eye and sense for historic details and as the lead maintainer of > libcurl I learn a lot here. It also keeps me motivated to provide this > documentation and work on keeping in accurate. > > Keep it up! <3 Thanks! And thanks for curl. For what it's worth I found myself looking for some bidirectional API mapping between these option IDs and their names, we tend to prefer that sort of pattern in git.git to macros, since we can test that something compiles everywhere, run tests without the feature without recompiling etc. I found that curl didn't have anything like that & started writing up a feature request for it, only to find that my curl.git checkout had been on an older commit, and that you'd added that API last year in curl.git 6ebe63fac (options: API for meta-data about easy options, 2020-08-26): https://curl.se/libcurl/c/curl_easy_option_by_name.html It's probably not something we can use in the next 10 years, but if whatever the most ancient OS we support upgrades past it it should make dealing with the version-specific code in http.c even easier.
diff --git a/http.c b/http.c index 56856178bfe..b82b5b7a532 100644 --- a/http.c +++ b/http.c @@ -732,7 +732,7 @@ static long get_curl_allowed_protocols(int from_user) return allowed_protocols; } -#if LIBCURL_VERSION_NUM >=0x072f00 +#if LIBCURL_VERSION_NUM >=0x072b00 static int get_curl_http_version_opt(const char *version_string, long *opt) { int i; @@ -774,7 +774,7 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2); } -#if LIBCURL_VERSION_NUM >= 0x072f00 // 7.47.0 +#if LIBCURL_VERSION_NUM >= 0x072b00 if (curl_http_version) { long opt; if (!get_curl_http_version_opt(curl_http_version, &opt)) {
In d73019feb44 (http: add support selecting http version, 2018-11-08) a dependency was added on CURL_HTTP_VERSION_2, but this feature was introduced in curl version 7.43.0, not 7.47.0, as the incorrect version check led us to believe. As looking through the history of that commit on the mailing list will reveal[1], the reason for this is that an earlier version of it depended on CURL_HTTP_VERSION_2TLS, which was introduced in libcurl 7.47.0. But the version that made it in in d73019feb44 had dropped the dependency on CURL_HTTP_VERSION_2TLS, but the corresponding version check was not corrected. The newest symbol we depend on is CURL_HTTP_VERSION_2. It was added in 7.33.0, but the CURL_HTTP_VERSION_2 alias we used was added in 7.47.0. So we could support an even older version here, but let's just correct the checked version. 1. https://lore.kernel.org/git/pull.69.git.gitgitgadget@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- http.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)