| Message ID | xmqqv8l9n5fj.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ci: do not die on deprecated-declarations warning | expand |
Hi Junio, On 14/01/2023 03:47, Junio C Hamano wrote: > Like a recent GitHub CI run on linux-musl [1] shows, we seem to be > getting a bunch of errors of the form: > > Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated: > since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR > [-Werror=deprecated-declarations] Yes, on 30-Dec-2022 I updated my cygwin installation, which updated the curl package(s) from v7.86.0 to v7.87.0, and caused my build to fail. I had a quick look at the new 'curl.h' header file and disabled the deprecation warnings to fix my build. I used vim to add the following to my config.mak: CFLAGS += -DCURL_DISABLE_DEPRECATION .. and then promptly forgot about it for a couple of weeks! :) As it happens, just last night I started to look at fixing the code to avoid the deprecated 'options'. However, if you are not interested in doing that, I can stop looking into that. ;) [I was probably doing it wrong anyway - I was just #ifdef-ing code based on the curl version number, rather than setting something up in the git-curl-compat.h header file; I haven't grokked that yet.] > > For some of them, it may be reasonable to follow the deprecation > notice and update the code, but some symbols like the above is not. > > According to the release table [2], 7.85.0 that deprecates > CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year > ago, and according to the symbols-in-versions table [3], > CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will > make us incompatible with anything older than a year if we rewrote > the call as the message suggests. > > Make sure that we won't break the build when -Wdeprecated-declarations > triggers. > > [1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050 > [2] https://curl.se/docs/releases.html > [3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > config.mak.dev | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/config.mak.dev b/config.mak.dev > index 981304727c..afcffa6a04 100644 > --- a/config.mak.dev > +++ b/config.mak.dev > @@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces > endif > endif > > +# Libraries deprecate symbols while retaining them for a long time to > +# keep software working with both older and newer versions of them. > +# Getting warnings does help the developers' awareness, but we cannot > +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR > +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS > +# but we cannot rewrite the uses of the latter with the former until > +# 7.85.0, which was released in August 2022, becomes ubiquitous. > +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations > + > # Old versions of clang complain about initializaing a > # struct-within-a-struct using just "{0}" rather than "{{0}}". This > # error is considered a false-positive and not worth fixing, because Rather than suppressing 'deprecated-declarations' globally, we could just add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c, http-push.c and remote-curl.c; similar to how we use target specific rules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar set of files. I haven't tried that yet (so famous last words...). Sorry for not looking into this (and notifying the list) sooner. ATB, Ramsay Jones
On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote: > Like a recent GitHub CI run on linux-musl [1] shows, we seem to be > getting a bunch of errors of the form: > > Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated: > since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR > [-Werror=deprecated-declarations] > > For some of them, it may be reasonable to follow the deprecation > notice and update the code, but some symbols like the above is not. Heh, I was just poking at this myself. > According to the release table [2], 7.85.0 that deprecates > CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year > ago, and according to the symbols-in-versions table [3], > CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will > make us incompatible with anything older than a year if we rewrote > the call as the message suggests. Yeah, we'd definitely have to keep code covering both versions for a while. Which is a good reason to punt until the deprecated option is actually removed, since enough time may have passed by then that we can avoid having to maintain both versions. The other thing that makes me care less about this deprecation is that the deprecation warning in the manpage says: We strongly recommend using CURLOPT_REDIR_PROTOCOLS_STR(3) instead because this option cannot control all available protocols! But we don't care about that. We are setting a very small and vanilla subset of the possible protocols, and the non-STR version is fine for that. > +# Libraries deprecate symbols while retaining them for a long time to > +# keep software working with both older and newer versions of them. > +# Getting warnings does help the developers' awareness, but we cannot > +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR > +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS > +# but we cannot rewrite the uses of the latter with the former until > +# 7.85.0, which was released in August 2022, becomes ubiquitous. > +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations That's a pretty broad hammer. And I think it may stomp on the hack to rely on deprecated() in the UNUSED macro. As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to limit this just to the problematic case. An even more focused option is to use curl's helper here: diff --git a/http.c b/http.c index 17d954dd95..21891493d9 100644 --- a/http.c +++ b/http.c @@ -1,3 +1,4 @@ +#define CURL_DISABLE_TYPECHECK 1 #include "git-compat-util.h" #include "git-curl-compat.h" #include "http.h" @@ -979,10 +980,12 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20); curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL); - curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, - get_curl_allowed_protocols(0)); - curl_easy_setopt(result, CURLOPT_PROTOCOLS, - get_curl_allowed_protocols(-1)); + CURL_IGNORE_DEPRECATION( + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, + get_curl_allowed_protocols(0)); + curl_easy_setopt(result, CURLOPT_PROTOCOLS, + get_curl_allowed_protocols(-1)); + ) if (getenv("GIT_CURL_VERBOSE")) http_trace_curl_no_data(); setup_curl_trace(result); though I think that was introduced only in 7.87.0 along with the deprecation warnings themselves, so we'd need to have a fallback like: #ifndef CURL_IGNORE_DEPRECATION(x) #define CURL_IGNORE_DEPRECATION(x) x #endif -Peff
On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote: > Like a recent GitHub CI run on linux-musl [1] shows, we seem to be > getting a bunch of errors of the form: > > Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated: > since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR > [-Werror=deprecated-declarations] By the way, it seemed odd to me that this failed in just the linux-musl job, and not elsewhere (nor on my development machine, which has curl 7.87.0). It looks like there's a bad interaction within curl between the typecheck and deprecation macros. Here's a minimal reproduction: -- >8 -- cat >foo.c <<-\EOF #include <curl/curl.h> void foo(CURL *c) { curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0); } EOF # this will complain about deprecated CURLOPT_PROTOCOLS gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c # this will not gcc -Wdeprecated-declarations -c foo.c -- 8< -- I didn't look into why the musl build behaves differently, but presumably it has an older compiler or something that causes curl to decide not to trigger the typecheck macros. Adding relevant curl folks to the cc (the curl list itself is subscriber-only). -Peff
On Sat, Jan 14, 2023 at 09:47:38AM -0500, Jeff King wrote: > As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to > limit this just to the problematic case. An even more focused option is > to use curl's helper here: > > diff --git a/http.c b/http.c > index 17d954dd95..21891493d9 100644 > --- a/http.c > +++ b/http.c > @@ -1,3 +1,4 @@ > +#define CURL_DISABLE_TYPECHECK 1 Oops, this line snuck in from my testing. See my other message for why it's relevant. ;) -Peff
On 14/01/2023 14:29, Ramsay Jones wrote: > Hi Junio, [snip] > Sorry for not looking into this (and notifying the list) sooner. Ah, that reminds me... When I updated to Linux Mint v21.0 (based on latest Ubuntu LTS) last year, sparse started failing. This was due to a change to the 'regex.h' header file (the libc6-dev package version was updated from 2.31 to 2.35), where (among other things) the diff looked like: .. 524a525,548 > #ifndef _REGEX_NELTS > # if (defined __STDC_VERSION__ && 199901L <= __STDC_VERSION__ \ > && !defined __STDC_NO_VLA__) > # define _REGEX_NELTS(n) n > # else > # define _REGEX_NELTS(n) > # endif > #endif > .. 645c681,682 < regmatch_t __pmatch[_Restrict_arr_], --- > regmatch_t __pmatch[_Restrict_arr_ > _REGEX_NELTS (__nmatch)], .. The last hunk is the declaration of regexec(), thus: extern int regexec (const regex_t *_Restrict_ __preg, const char *_Restrict_ __String, size_t __nmatch, regmatch_t __pmatch[_Restrict_arr_ _REGEX_NELTS (__nmatch)], int __eflags); So, sparse falls over on the '__nmatch' as part of the __pmatch argument declaration. [Actually, it doesn't bomb out on the declaration, but at each regexec() call site]. To fix my build, I added the following to my config.mak file on linux: SPARSE_FLAGS += -D__STDC_NO_VLA__ .. and forgot about it! :) I need to fix this sometime. ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > Yes, on 30-Dec-2022 I updated my cygwin installation, which updated > the curl package(s) from v7.86.0 to v7.87.0, and caused my build > to fail. I had a quick look at the new 'curl.h' header file and > disabled the deprecation warnings to fix my build. I used vim to > add the following to my config.mak: > > CFLAGS += -DCURL_DISABLE_DEPRECATION > > .. and then promptly forgot about it for a couple of weeks! :) Ahh, this is why I love to be working with this community, where I can float a problem with a possible solution and fellow developers respond with better solutions very quickly. >> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations >> + > > Rather than suppressing 'deprecated-declarations' globally, we could > just add '-DCURL_DISABLE_DEPRECATION' to the compilation of http.c, > http-push.c and remote-curl.c; similar to how we use target specific > rules to pass '-DCURL_DISABLE_TYPECHECK' to sparse (only) to a similar > set of files. OK, or, assuming that non-curl including code will not be affected with the macro, we can globally add -DCURL_DISABLE_DEPRECATION to CFLAGS or DEVELOPER_CFLAGS. I guess I can sit back and wait until a better version materializes ;-)? Thanks.
Jeff King <peff@peff.net> writes: >> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations > > That's a pretty broad hammer. And I think it may stomp on the hack to > rely on deprecated() in the UNUSED macro. > > As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to > limit this just to the problematic case. Yeah, I like that approach. But not this one ... > + CURL_IGNORE_DEPRECATION( > + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, > + get_curl_allowed_protocols(0)); > + curl_easy_setopt(result, CURLOPT_PROTOCOLS, > + get_curl_allowed_protocols(-1)); > + ) > > though I think that was introduced only in 7.87.0 along with the > deprecation warnings themselves, so we'd need to have a fallback like: > > #ifndef CURL_IGNORE_DEPRECATION(x) > #define CURL_IGNORE_DEPRECATION(x) x > #endif ... as much.
Jeff King <peff@peff.net> writes: >> +# Libraries deprecate symbols while retaining them for a long time to >> +# keep software working with both older and newer versions of them. >> +# Getting warnings does help the developers' awareness, but we cannot >> +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR >> +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS >> +# but we cannot rewrite the uses of the latter with the former until >> +# 7.85.0, which was released in August 2022, becomes ubiquitous. >> +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations > > That's a pretty broad hammer. And I think it may stomp on the hack to > rely on deprecated() in the UNUSED macro. True. > As Ramsay suggested, we could probably use CURL_DISABLE_DEPRECATION to > limit this just to the problematic case. An even more focused option is > to use curl's helper here: One possible downside of the use of CURL_DISABLE_DEPRECATION is that we may still want to see deprecation warning to learn about upcoming change. We just do not want the -Werror to make us die when it happens. But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it goes. Thanks.
On Sat, Jan 14, 2023 at 08:15:21AM -0800, Junio C Hamano wrote: > Yeah, I like that approach. > > But not this one ... > > > + CURL_IGNORE_DEPRECATION( > > + curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, > > + get_curl_allowed_protocols(0)); > > + curl_easy_setopt(result, CURLOPT_PROTOCOLS, > > + get_curl_allowed_protocols(-1)); > > + ) > > > > though I think that was introduced only in 7.87.0 along with the > > deprecation warnings themselves, so we'd need to have a fallback like: > > > > #ifndef CURL_IGNORE_DEPRECATION(x) > > #define CURL_IGNORE_DEPRECATION(x) x > > #endif > > ... as much. I agree it's pretty ugly. The one advantage it has is that we'd be informed of _other_ curl deprecations that might be more interesting to us. On the other hand, I don't know how useful those deprecations are going to be, as it depends on the timeframe. If the deprecation is added for the same version of libcurl that implements the alternative (which is roughly the case here), then we'd always be stuck supporting the old and new forms (old for backwards compatibility, and new to silence the deprecation warning). We care a lot more about the deprecation once the alternative has been around for a while, and/or the old way of doing things is about to be removed. And if we just wait until that removal, then we do not have to rely on deprecation warnings. The build will break just fine on its own. :) -Peff
Jeff King <peff@peff.net> writes: > On the other hand, I don't know how useful those deprecations are going > to be, as it depends on the timeframe. If the deprecation is added for > the same version of libcurl that implements the alternative (which is > roughly the case here), then we'd always be stuck supporting the old and > new forms (old for backwards compatibility, and new to silence the > deprecation warning). ... or just keep the warning without promoting, with "-Wno-error=...". > We care a lot more about the deprecation once the > alternative has been around for a while, and/or the old way of doing > things is about to be removed. And if we just wait until that removal, > then we do not have to rely on deprecation warnings. The build will > break just fine on its own. :) Yes and no. It is not always like "this symbol is now known under this different name", which is trivial to adjust. I briefly tried to see how IOCTL -> SEEK change should look like until I realized that the new way was invented too recently and stopped looking, but it would involve changes to the function logic in the callback functions, as the function signature---both parameters and its return values---of the callback changes. I do not want to see us scrambling to make such adjustments to the code at the last minute, so some sort of advance warning is a good thing to have. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it > goes. The "DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations" version was merged to 'next', only because I wanted to see the commit cleanly pass the tests (and it does), but I do think in the longer term (like, before the topic hits 'master'), it probably is better to do this for everybody, not just for those who use DEVELOPER=Yes. So, further patches on top are very much welcomed. Thanks.
On Sat, Jan 14, 2023 at 10:59:18PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On the other hand, I don't know how useful those deprecations are going > > to be, as it depends on the timeframe. If the deprecation is added for > > the same version of libcurl that implements the alternative (which is > > roughly the case here), then we'd always be stuck supporting the old and > > new forms (old for backwards compatibility, and new to silence the > > deprecation warning). > > ... or just keep the warning without promoting, with "-Wno-error=...". I'm pretty strongly against that solution for anything long-term. Having to see those warnings is not only ugly, but it's confusing and trains people to ignore them (and many times I've noticed ugly warnings floating by and realized that oops, I had temporarily disabled -Werror because I had been working with some older code). > > We care a lot more about the deprecation once the > > alternative has been around for a while, and/or the old way of doing > > things is about to be removed. And if we just wait until that removal, > > then we do not have to rely on deprecation warnings. The build will > > break just fine on its own. :) > > Yes and no. It is not always like "this symbol is now known under > this different name", which is trivial to adjust. I briefly tried > to see how IOCTL -> SEEK change should look like until I realized > that the new way was invented too recently and stopped looking, but > it would involve changes to the function logic in the callback > functions, as the function signature---both parameters and its > return values---of the callback changes. I do not want to see us > scrambling to make such adjustments to the code at the last minute, > so some sort of advance warning is a good thing to have. True. I do think the IOCTL/SEEK one is old enough that we can do, though. The deprecation is newer, but the SEEK interface was added in an old enough version. -Peff
On Sat, Jan 14, 2023 at 11:02:32PM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > But anyway, let's use CURL_DISABLE_DEPRECATION first to see how it > > goes. > > The "DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations" version > was merged to 'next', only because I wanted to see the commit > cleanly pass the tests (and it does), but I do think in the longer > term (like, before the topic hits 'master'), it probably is better > to do this for everybody, not just for those who use DEVELOPER=Yes. > > So, further patches on top are very much welcomed. So I took a look at just dropping the deprecated bits, and it wasn't _too_ bad. Here's that series. The first two I hope are obviously good. The third one is _ugly_, but at least it punts on the whole "how should we silence this" argument, and it takes us in the direction we'd ultimately want to go. [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION [3/3]: http: support CURLOPT_PROTOCOLS_STR git-curl-compat.h | 8 +++++++ http-push.c | 6 ++--- http.c | 61 +++++++++++++++++++++++++++++++++-------------- http.h | 2 +- remote-curl.c | 28 ++++++++++------------ 5 files changed, 68 insertions(+), 37 deletions(-)
Jeff King <peff@peff.net> writes: > I do think the IOCTL/SEEK one is old enough that we can do, though. The > deprecation is newer, but the SEEK interface was added in an old enough > version. Yes, that one is old enough (SEEKFUNCTION and frieds are from 7.18.0 and 7.19.5, IOCTL was deprecated at 7.18.0, released 15 years ago). A problematic one is REDIR_PROTOCOLS that was deprecated in 7.85.0 (Aug 2022) whose replacement appeared in the same 7.85.0 version. Thanks.
On 14/01/2023 14:56, Jeff King wrote: > On Fri, Jan 13, 2023 at 07:47:12PM -0800, Junio C Hamano wrote: > >> Like a recent GitHub CI run on linux-musl [1] shows, we seem to be >> getting a bunch of errors of the form: >> >> Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated: >> since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR >> [-Werror=deprecated-declarations] > > By the way, it seemed odd to me that this failed in just the linux-musl > job, and not elsewhere (nor on my development machine, which has curl > 7.87.0). It looks like there's a bad interaction within curl between the > typecheck and deprecation macros. Here's a minimal reproduction: > > -- >8 -- > cat >foo.c <<-\EOF > #include <curl/curl.h> > void foo(CURL *c) > { > curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0); > } > EOF > > # this will complain about deprecated CURLOPT_PROTOCOLS > gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c > > # this will not > gcc -Wdeprecated-declarations -c foo.c > -- 8< -- FYI, I just tried this on cygwin and both gcc invocations above complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have curl 7.81.0, so I can't test there). [cygwin uses newlib, of course]. ATB, Ramsay Jones
On Mon, Jan 16, 2023 at 12:39:39AM +0000, Ramsay Jones wrote: > > By the way, it seemed odd to me that this failed in just the linux-musl > > job, and not elsewhere (nor on my development machine, which has curl > > 7.87.0). It looks like there's a bad interaction within curl between the > > typecheck and deprecation macros. Here's a minimal reproduction: > > > > -- >8 -- > > cat >foo.c <<-\EOF > > #include <curl/curl.h> > > void foo(CURL *c) > > { > > curl_easy_setopt(c, CURLOPT_PROTOCOLS, 0); > > } > > EOF > > > > # this will complain about deprecated CURLOPT_PROTOCOLS > > gcc -DCURL_DISABLE_TYPECHECK -Wdeprecated-declarations -c foo.c > > > > # this will not > > gcc -Wdeprecated-declarations -c foo.c > > -- 8< -- > > FYI, I just tried this on cygwin and both gcc invocations above > complain about deprecated CURLOPT_PROTOCOLS. (On Linux I have > curl 7.81.0, so I can't test there). It did work on Linux for me, of course, using 7.87.0. But curiously this morning it behaved differently! In the meantime, Debian's libcurl packaging picked up this upstream patch (and I upgraded): https://github.com/curl/curl/commit/e2aed004302e51cfa5b6ce8c8ab65ef92aa83196 and now the deprecation warning happens consistently. So I think on the curl side there is nothing left to do. -Peff
On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote: > So I took a look at just dropping the deprecated bits, and it wasn't > _too_ bad. Here's that series. The first two I hope are obviously good. > The third one is _ugly_, but at least it punts on the whole "how should > we silence this" argument, and it takes us in the direction we'd > ultimately want to go. > > [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT > [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION > [3/3]: http: support CURLOPT_PROTOCOLS_STR In the interests of wrapping this up, here's a v2 that: - bumps the required curl version to 7.19.5 in patch 2 - aims for slightly better readability in the final code of patch 3, versus minimizing the diff As discussed, there are a lot of different ways one could do patch 3, but I really don't think it's worth spending that much brain power on. What's here is correct (most important), and I think should be easy to clean up when we can eventually drop the old style. [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION [3/3]: http: support CURLOPT_PROTOCOLS_STR INSTALL | 2 +- git-curl-compat.h | 8 +++++ http-push.c | 6 ++-- http.c | 79 +++++++++++++++++++++++++++++++++-------------- http.h | 2 +- remote-curl.c | 28 ++++++++--------- 6 files changed, 81 insertions(+), 44 deletions(-) 1: 2229c0468f = 1: 5ae6831af5 http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT 2: 00120fa40e ! 2: 5be76d74de http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION @@ Commit message instead. It was added in 2008 via curl 7.18.0; our INSTALL file already indicates we require at least curl 7.19.4. - We have to rewrite the ioctl functions into seek functions. In some ways - they are simpler (seeking is the only operation), but in some ways more - complex (the ioctl allowed only a full rewind, but now we can seek to - arbitrary offsets). + But there's one catch: curl says we should use CURL_SEEKFUNC_{OK,FAIL}, + and those didn't arrive until 7.19.5. One workaround would be to use a + bare 0/1 here (or define our own macros). But let's just bump the + minimum required version to 7.19.5. That version is only a minor version + bump from our existing requirement, and is only a 2 month time bump for + versions that are almost 13 years old. So it's not likely that anybody + cares about the distinction. + + Switching means we have to rewrite the ioctl functions into seek + functions. In some ways they are simpler (seeking is the only + operation), but in some ways more complex (the ioctl allowed only a full + rewind, but now we can seek to arbitrary offsets). Curl will only ever use SEEK_SET (per their documentation), so I didn't bother implementing anything else, since it would naturally be @@ Commit message Signed-off-by: Jeff King <peff@peff.net> + ## INSTALL ## +@@ INSTALL: Issues of note: + not need that functionality, use NO_CURL to build without + it. + +- Git requires version "7.19.4" or later of "libcurl" to build ++ Git requires version "7.19.5" or later of "libcurl" to build + without NO_CURL. This version requirement may be bumped in + the future. + + ## http-push.c ## @@ http-push.c: static void curl_setup_http(CURL *curl, const char *url, curl_easy_setopt(curl, CURLOPT_INFILE, buffer); 3: 22eb2fd0fe ! 3: d2c28e22e1 http: support CURLOPT_PROTOCOLS_STR @@ http.c: void setup_curl_trace(CURL *handle) } -static long get_curl_allowed_protocols(int from_user) -+static void proto_list_append(struct strbuf *list_str, const char *proto_str, -+ long *list_bits, long proto_bits) -+{ -+ *list_bits |= proto_bits; -+ if (list_str) { -+ if (list_str->len) -+ strbuf_addch(list_str, ','); -+ strbuf_addstr(list_str, proto_str); -+ } -+} -+ -+static long get_curl_allowed_protocols(int from_user, struct strbuf *list) ++static void proto_list_append(struct strbuf *list, const char *proto) { - long allowed_protocols = 0; +- long allowed_protocols = 0; ++ if (!list) ++ return; ++ if (list->len) ++ strbuf_addch(list, ','); ++ strbuf_addstr(list, proto); ++} - if (is_transport_allowed("http", from_user)) +- if (is_transport_allowed("http", from_user)) - allowed_protocols |= CURLPROTO_HTTP; -+ proto_list_append(list, "http", &allowed_protocols, CURLPROTO_HTTP); - if (is_transport_allowed("https", from_user)) +- if (is_transport_allowed("https", from_user)) - allowed_protocols |= CURLPROTO_HTTPS; -+ proto_list_append(list, "https", &allowed_protocols, CURLPROTO_HTTPS); - if (is_transport_allowed("ftp", from_user)) +- if (is_transport_allowed("ftp", from_user)) - allowed_protocols |= CURLPROTO_FTP; -+ proto_list_append(list, "ftp", &allowed_protocols, CURLPROTO_FTP); - if (is_transport_allowed("ftps", from_user)) +- if (is_transport_allowed("ftps", from_user)) - allowed_protocols |= CURLPROTO_FTPS; -+ proto_list_append(list, "ftps", &allowed_protocols, CURLPROTO_FTPS); ++static long get_curl_allowed_protocols(int from_user, struct strbuf *list) ++{ ++ long bits = 0; - return allowed_protocols; +- return allowed_protocols; ++ if (is_transport_allowed("http", from_user)) { ++ bits |= CURLPROTO_HTTP; ++ proto_list_append(list, "http"); ++ } ++ if (is_transport_allowed("https", from_user)) { ++ bits |= CURLPROTO_HTTPS; ++ proto_list_append(list, "https"); ++ } ++ if (is_transport_allowed("ftp", from_user)) { ++ bits |= CURLPROTO_FTP; ++ proto_list_append(list, "ftp"); ++ } ++ if (is_transport_allowed("ftps", from_user)) { ++ bits |= CURLPROTO_FTPS; ++ proto_list_append(list, "ftps"); ++ } ++ ++ return bits; } + + #ifdef GIT_CURL_HAVE_CURL_HTTP_VERSION_2 @@ http.c: static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
On 17/01/2023 03:03, Jeff King wrote: > On Sun, Jan 15, 2023 at 03:09:26PM -0500, Jeff King wrote: > >> So I took a look at just dropping the deprecated bits, and it wasn't >> _too_ bad. Here's that series. The first two I hope are obviously good. >> The third one is _ugly_, but at least it punts on the whole "how should >> we silence this" argument, and it takes us in the direction we'd >> ultimately want to go. >> >> [1/3]: http-push: prefer CURLOPT_UPLOAD to CURLOPT_PUT >> [2/3]: http: prefer CURLOPT_SEEKFUNCTION to CURLOPT_IOCTLFUNCTION >> [3/3]: http: support CURLOPT_PROTOCOLS_STR > > In the interests of wrapping this up, here's a v2 that: > > - bumps the required curl version to 7.19.5 in patch 2 > > - aims for slightly better readability in the final code of patch 3, > versus minimizing the diff > I have a _slight_ preference for your v1 patches, but I don't hate this version either! :) Tonight, I have compile tested both v1 and v2 patches (both in 'seen') on cygwin and linux. I would like to say I have run the tests as well, but it seems I have disabled all the tests on cygwin (expected) *and* on linux (most unexpected). ie. I don't have apache installed. (I used to have apache, svn and cvs installed to run the tests, but they just took too long to run and caused *many* test runs to hang and leave server processes all over the place. On cygwin, even with all of those tests skipped, it still takes approx 5.5 hours to run the tests). I thought I was still running the '*http*' tests on linux, but I seem to have dropped the installation of apache at some point - oops! I guess I should install apache tomorrow ... ATB, Ramsay Jones
diff --git a/config.mak.dev b/config.mak.dev index 981304727c..afcffa6a04 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -69,6 +69,15 @@ DEVELOPER_CFLAGS += -Wno-missing-braces endif endif +# Libraries deprecate symbols while retaining them for a long time to +# keep software working with both older and newer versions of them. +# Getting warnings does help the developers' awareness, but we cannot +# afford to update too aggressively. E.g. CURLOPT_REDIR_PROTOCOLS_STR +# is only available in 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS +# but we cannot rewrite the uses of the latter with the former until +# 7.85.0, which was released in August 2022, becomes ubiquitous. +DEVELOPER_CFLAGS += -Wno-error=deprecated-declarations + # Old versions of clang complain about initializaing a # struct-within-a-struct using just "{0}" rather than "{{0}}". This # error is considered a false-positive and not worth fixing, because
Like a recent GitHub CI run on linux-musl [1] shows, we seem to be getting a bunch of errors of the form: Error: http.c:1002:9: 'CURLOPT_REDIR_PROTOCOLS' is deprecated: since 7.85.0. Use CURLOPT_REDIR_PROTOCOLS_STR [-Werror=deprecated-declarations] For some of them, it may be reasonable to follow the deprecation notice and update the code, but some symbols like the above is not. According to the release table [2], 7.85.0 that deprecates CURLOPT_REDIR_PROTOCOLS was released on 2022-08-31, less than a year ago, and according to the symbols-in-versions table [3], CURLOPT_REDIR_PROTOCOLS_STR was introduced in 7.85.0, so it will make us incompatible with anything older than a year if we rewrote the call as the message suggests. Make sure that we won't break the build when -Wdeprecated-declarations triggers. [1] https://github.com/git/git/actions/runs/3915509922/jobs/6693756050 [2] https://curl.se/docs/releases.html [3] https://github.com/curl/curl/blob/master/docs/libcurl/symbols-in-versions Signed-off-by: Junio C Hamano <gitster@pobox.com> --- config.mak.dev | 9 +++++++++ 1 file changed, 9 insertions(+)