| Message ID | 20240621154552.62038-2-randall.becker@nexbridge.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Teach git version --build-options about zlib+libcurl | expand |
"Randall S. Becker" <the.n.e.key@gmail.com> writes: > This change uses the zlib supplied ZLIB_VERSION #define supplied text > macro and the libcurl LIBCURL_VERSION #define text macro. No > stringification is required for either variable's use. If either of > the #define is not present, that version is not reported. "the zlib supplied ZLIB_VERSION #define supplied text macro" is quite a mouthful. Something like version: --build-options reports zlib and libcurl version information Use ZLIB_VERSION and LIBCURL_VERSION to show them, if defined, in "git version --build-options" output. should be sufficient. We will assume that (1) LIBFROTZ_VERSION, if defined, will always be of the same type (luckily, all three we are dealing with use a C-string so "strbuf_addf(buf, "%s", LIBFROTZ_VERSION)" is good), and that (2) no random origin other than the frotz project will define the CPP macro LIBFROTZ_VERSION to confuse us. Both are sensible assumptions that would allow us to trust a hardcoded strbuf_addf() invocation per each library is sufficient If a library uses LIBFROTZ_MAJOR and LIBFROTZ_MINOR we may have to do "strbuf_addf(buf, "%s.%s" LIBFROTZ_MAJOR, LIBFROTZ_MINOR)" that is different from others, but the point is the version identification scheme would be constant across different versions of the same library. The actual code to report versions should be trivial, once we get the mechanism to make necessary CPP macros available (when present) right, but the latter needs a bit more work than this patch shows. Here is the first change your patch does: > #include "git-compat-util.h" > +#include "git-curl-compat.h" The file <git-curl-compat.h> begins like so: #ifndef GIT_CURL_COMPAT_H #define GIT_CURL_COMPAT_H #include <curl/curl.h> ... If you do not have any <curl/curl.h> anywhere on your system, I suspect this will break the build, instead of silently leaving LIBCURL_VERSION undefined. > #include "config.h" > #include "builtin.h" > #include "exec-cmd.h" > @@ -757,6 +758,12 @@ void get_version_info(struct strbuf *buf, int show_build_options) > > if (fsmonitor_ipc__is_supported()) > strbuf_addstr(buf, "feature: fsmonitor--daemon\n"); > +#if defined LIBCURL_VERSION > + strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION); > +#endif > +#if defined ZLIB_VERSION > + strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION); > +#endif FYI, in the merged result, I would prefer to order these entries semi-alphabetically, e.g. perhaps stripping possible "lib" prefix or suffix and comparing the rest to result in curl < openssl < z or something like that. Then we know where to add a new one, whose name we do not know yet, in the future. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > "the zlib supplied ZLIB_VERSION #define supplied text macro" is > quite a mouthful. Something like > > version: --build-options reports zlib and libcurl version information > > Use ZLIB_VERSION and LIBCURL_VERSION to show them, if defined, in > "git version --build-options" output. > > should be sufficient. Nah, that is still more verbose than necessary. Just saying Show ZLIB_VERSION and LIBCURL_VERSION, if defined, in ... is sufficient.
On Friday, June 21, 2024 2:20 PM, Junio C Hamano wrote: >"Randall S. Becker" <the.n.e.key@gmail.com> writes: > >> This change uses the zlib supplied ZLIB_VERSION #define supplied text >> macro and the libcurl LIBCURL_VERSION #define text macro. No >> stringification is required for either variable's use. If either of >> the #define is not present, that version is not reported. > >"the zlib supplied ZLIB_VERSION #define supplied text macro" is quite a mouthful. >Something like > > version: --build-options reports zlib and libcurl version information > > Use ZLIB_VERSION and LIBCURL_VERSION to show them, if defined, in > "git version --build-options" output. > >should be sufficient. Do you want me to reissue the merge? This looks fine to me. >We will assume that > > (1) LIBFROTZ_VERSION, if defined, will always be of the same type > (luckily, all three we are dealing with use a C-string so > "strbuf_addf(buf, "%s", LIBFROTZ_VERSION)" is good), and that > > (2) no random origin other than the frotz project will define the > CPP macro LIBFROTZ_VERSION to confuse us. > >Both are sensible assumptions that would allow us to trust a hardcoded >strbuf_addf() invocation per each library is sufficient If a library uses >LIBFROTZ_MAJOR and LIBFROTZ_MINOR we may have to do "strbuf_addf(buf, >"%s.%s" LIBFROTZ_MAJOR, LIBFROTZ_MINOR)" that is different from others, but >the point is the version identification scheme would be constant across different >versions of the same library. > >The actual code to report versions should be trivial, once we get the mechanism to >make necessary CPP macros available (when present) right, but the latter needs a >bit more work than this patch shows. > >Here is the first change your patch does: > >> #include "git-compat-util.h" >> +#include "git-curl-compat.h" > >The file <git-curl-compat.h> begins like so: > > #ifndef GIT_CURL_COMPAT_H > #define GIT_CURL_COMPAT_H > #include <curl/curl.h> > ... > In this case, I was modelling the include after http.c, and remote-curl.c, which would have the same problem. I was going for consistency. Would not all three have to be fixed in a separate patch? >If you do not have any <curl/curl.h> anywhere on your system, I suspect this will >break the build, instead of silently leaving LIBCURL_VERSION undefined. > >> #include "config.h" >> #include "builtin.h" >> #include "exec-cmd.h" >> @@ -757,6 +758,12 @@ void get_version_info(struct strbuf *buf, int >> show_build_options) >> >> if (fsmonitor_ipc__is_supported()) >> strbuf_addstr(buf, "feature: fsmonitor--daemon\n"); >> +#if defined LIBCURL_VERSION >> + strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION); #endif #if >> +defined ZLIB_VERSION >> + strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION); #endif > >FYI, in the merged result, I would prefer to order these entries semi-alphabetically, >e.g. perhaps stripping possible "lib" prefix or suffix and comparing the rest to result >in curl < openssl < z or something like that. Then we know where to add a new one, >whose name we do not know yet, in the future. I think that is logical. Do you need this redone? Although the OpenSSL inclusion is already merged from what I can see. >Thanks.
<rsbecker@nexbridge.com> writes: > In this case, I was modelling the include after http.c, and remote-curl.c, > which would have the same problem. I was going for consistency. Would not > all three have to be fixed in a separate patch? At least for build on platforms without libcURL, you build with NO_CURL defined, i.e. "make NO_CURL=NoThanks", and anything that includes <curl/curl.h> is *NOT* compiled at all, avoiding the broken build. There is *NOTHING* that needs fixing in the existing code. Only this patch under discussion is buggy that way. >>FYI, in the merged result, I would prefer to order these entries > semi-alphabetically, >>e.g. perhaps stripping possible "lib" prefix or suffix and comparing the > rest to result >>in curl < openssl < z or something like that. Then we know where to add a > new one, >>whose name we do not know yet, in the future. > > I think that is logical. Do you need this redone? Although the OpenSSL > inclusion is already merged from what I can see. That is why the statement has "FYI". I'll do the merging. Having them as two patches, one for libcurl and the other for zlib, would be slightly cleaner. Otherwise my merge would have to become "splitting the new one that adds libcurl+zlib into two hunks and let the existing openssl one in between".
Junio C Hamano <gitster@pobox.com> writes: > At least for build on platforms without libcURL, you build with > NO_CURL defined, i.e. "make NO_CURL=NoThanks", and anything that > includes <curl/curl.h> is *NOT* compiled at all, avoiding the broken > build. Unfortunately, we cannot use the same trick, i.e. "Makefile knows not to even compile when NO_CURL is set", as this change is to help.c and we cannot say "if you do not have libcURL, you do not get any help" ;-) #ifndef NO_CURL #include "git-curl-compat.h" #endif may be a simplest workaround, as Makefile does this: ifdef NO_CURL BASIC_CFLAGS += -DNO_CURL ...
On Friday, June 21, 2024 3:20 PM, Junio C Hamano wrote: >To: rsbecker@nexbridge.com >Cc: 'Randall S. Becker' <the.n.e.key@gmail.com>; git@vger.kernel.org; Randall >Becker <randall.becker@nexbridge.ca> >Subject: Re: [PATCH v0 1/1] Teach git version --build-options about zlib+libcurl > >Junio C Hamano <gitster@pobox.com> writes: > >> At least for build on platforms without libcURL, you build with >> NO_CURL defined, i.e. "make NO_CURL=NoThanks", and anything that >> includes <curl/curl.h> is *NOT* compiled at all, avoiding the broken >> build. > >Unfortunately, we cannot use the same trick, i.e. "Makefile knows not to even >compile when NO_CURL is set", as this change is to help.c and we cannot say "if you >do not have libcURL, you do not get any help" ;-) > > #ifndef NO_CURL > #include "git-curl-compat.h" > #endif > >may be a simplest workaround, as Makefile does this: > > ifdef NO_CURL > BASIC_CFLAGS += -DNO_CURL > ... That makes sense
Junio C Hamano <gitster@pobox.com> writes: > Unfortunately, we cannot use the same trick, i.e. "Makefile > knows not to even compile when NO_CURL is set", as this change is to > help.c and we cannot say "if you do not have libcURL, you do not get > any help" ;-) > > #ifndef NO_CURL > #include "git-curl-compat.h" > #endif > > may be a simplest workaround, as Makefile does this: > > ifdef NO_CURL > BASIC_CFLAGS += -DNO_CURL > ... So, the version I queued looks like so: diff --git a/help.c b/help.c index ce55aaa2c0..92bfef140b 100644 --- a/help.c +++ b/help.c @@ -15,6 +15,10 @@ #include "prompt.h" #include "fsmonitor-ipc.h" +#ifndef NO_CURL +#include "git-curl-compat.h" /* For LIBCURL_VERSION only */ +#endif + struct category_description { uint32_t category; const char *desc; @@ -757,6 +761,9 @@ void get_version_info(struct strbuf ... if (fsmonitor_ipc__is_supported()) strbuf_addstr(buf, "feature: fsmonitor--daemon\n"); +#if defined LIBCURL_VERSION + strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION); +#endif #if defined OPENSSL_VERSION_TEXT strbuf_addf(buf, "OpenSSL: %s\n", OPENSSL_VERSION_TEXT); #endif but then there are a few "side builds" at GitHub CI, one of which is "minimum fuzzer" build. It compiles bunch of object files without giving much build options but the final target of the build is not "git" but something else [*]. And because the job is not interesting in building a working "git", the environment does not install libcURL, leading to a failed build. I sent a separate patch to address this build failure, which is found at https://lore.kernel.org/git/xmqqwmmhimxx.fsf@gitster.g/ [Reference] * https://github.com/git/git/actions/runs/9623017127/job/26544995557
On Saturday, June 22, 2024 1:11 AM, Junio C Hamano wrote: >Junio C Hamano <gitster@pobox.com> writes: > >> Unfortunately, we cannot use the same trick, i.e. "Makefile knows not >> to even compile when NO_CURL is set", as this change is to help.c and >> we cannot say "if you do not have libcURL, you do not get any help" >> ;-) >> >> #ifndef NO_CURL >> #include "git-curl-compat.h" >> #endif >> >> may be a simplest workaround, as Makefile does this: >> >> ifdef NO_CURL >> BASIC_CFLAGS += -DNO_CURL >> ... > >So, the version I queued looks like so: > > diff --git a/help.c b/help.c > index ce55aaa2c0..92bfef140b 100644 > --- a/help.c > +++ b/help.c > @@ -15,6 +15,10 @@ > #include "prompt.h" > #include "fsmonitor-ipc.h" > > +#ifndef NO_CURL > +#include "git-curl-compat.h" /* For LIBCURL_VERSION only */ > +#endif > + > struct category_description { > uint32_t category; > const char *desc; > @@ -757,6 +761,9 @@ void get_version_info(struct strbuf ... > > if (fsmonitor_ipc__is_supported()) > strbuf_addstr(buf, "feature: fsmonitor--daemon\n"); > +#if defined LIBCURL_VERSION > + strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION); > +#endif > #if defined OPENSSL_VERSION_TEXT > strbuf_addf(buf, "OpenSSL: %s\n", OPENSSL_VERSION_TEXT); > #endif > >but then there are a few "side builds" at GitHub CI, one of which is "minimum >fuzzer" build. It compiles bunch of object files without giving much build options >but the final target of the build is not "git" but something else [*]. And because the >job is not interesting in building a working "git", the environment does not install >libcURL, leading to a failed build. > >I sent a separate patch to address this build failure, which is found at >https://lore.kernel.org/git/xmqqwmmhimxx.fsf@gitster.g/ My take on the separate patches and discussion about reporting run-time values of libcurl, zlib, and OpenSSL, is that these are being added to --build-options not --runtime-options (does not exist yet). I think that grabbing run-time values could be confusing to users who expect the --build-options even if comparing the two values. --Randall
<rsbecker@nexbridge.com> writes: > My take on the separate patches and discussion about reporting run-time > values of libcurl, zlib, and OpenSSL, is that these are being added to > --build-options not --runtime-options (does not exist yet). I think that > grabbing run-time values could be confusing to users who expect the > --build-options even if comparing the two values. Yup. I thought that the consensus was to leave all those extra complexities like runtime versions for a later and separate topic done after the dust from this change settles. Thanks.
On Tuesday, June 25, 2024 4:58 PM, Junio C Hamano wrote: ><rsbecker@nexbridge.com> writes: > >> My take on the separate patches and discussion about reporting >> run-time values of libcurl, zlib, and OpenSSL, is that these are being >> added to --build-options not --runtime-options (does not exist yet). I >> think that grabbing run-time values could be confusing to users who >> expect the --build-options even if comparing the two values. > >Yup. I thought that the consensus was to leave all those extra complexities like >runtime versions for a later and separate topic done after the dust from this change >settles. So did I, but there was other chatter that made me think we did not.
On Tue, Jun 25, 2024 at 01:58:18PM -0700, Junio C Hamano wrote: > <rsbecker@nexbridge.com> writes: > > > My take on the separate patches and discussion about reporting run-time > > values of libcurl, zlib, and OpenSSL, is that these are being added to > > --build-options not --runtime-options (does not exist yet). I think that > > grabbing run-time values could be confusing to users who expect the > > --build-options even if comparing the two values. > > Yup. I thought that the consensus was to leave all those extra > complexities like runtime versions for a later and separate topic > done after the dust from this change settles. My only qualm is that reading curl.h at all in a program that is not going to link it feels a bit funny (it is declaring symbols that will not be available at link time). And we could fix that immediately by having "remote-https --build-options". Adding a curl_version() check on top of that could come later, but of course it would be easy to do. But that may just be me being overly conservative. If nobody looks at those symbols, I'm not sure what harm would come. -Peff
On Tue, Jun 25, 2024 at 03:29:03PM -0400, rsbecker@nexbridge.com wrote: > My take on the separate patches and discussion about reporting run-time > values of libcurl, zlib, and OpenSSL, is that these are being added to > --build-options not --runtime-options (does not exist yet). I think that > grabbing run-time values could be confusing to users who expect the > --build-options even if comparing the two values. I think that's a valid concern. I assumed we'd show both runtime and build-time values side by side, clearly labeled, rather than replace one with the other. I don't see much need for --runtime-options. The point of --build-options is to gather info about the binary for debugging. While technically you can swap out the dynamic library whenever you like, in practice I think it is about asking "what will happen now when I run git". Likewise, you could answer that part with "ldd git", but this is about making it simpler to gather the information in one place. -Peff
Jeff King <peff@peff.net> writes: > But that may just be me being overly conservative. If nobody looks at > those symbols, I'm not sure what harm would come. Yeah, perhaps on some exotic systems it may cause problems, but until we see such a system, or we want to extend it to do the curl_version() call for runtime, whichever comes earlier, I think we can leave it as a (#leftoverbits) "future enhancement and clean-up" task to be done after the dust settles. Thanks.
diff --git a/help.c b/help.c index 1d057aa607..f378750af4 100644 --- a/help.c +++ b/help.c @@ -1,4 +1,5 @@ #include "git-compat-util.h" +#include "git-curl-compat.h" #include "config.h" #include "builtin.h" #include "exec-cmd.h" @@ -757,6 +758,12 @@ void get_version_info(struct strbuf *buf, int show_build_options) if (fsmonitor_ipc__is_supported()) strbuf_addstr(buf, "feature: fsmonitor--daemon\n"); +#if defined LIBCURL_VERSION + strbuf_addf(buf, "libcurl: %s\n", LIBCURL_VERSION); +#endif +#if defined ZLIB_VERSION + strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION); +#endif } }
This change uses the zlib supplied ZLIB_VERSION #define supplied text macro and the libcurl LIBCURL_VERSION #define text macro. No stringification is required for either variable's use. If either of the #define is not present, that version is not reported. Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com> --- help.c | 7 +++++++ 1 file changed, 7 insertions(+)