On Friday, June 21, 2024 2:20 PM, Junio C Hamano wrote: >"Randall S. Becker" <the.n.e.key@xxxxxxxxx> 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.
