RE: [PATCH v0 1/1] Teach git version --build-options about zlib+libcurl

[<rsbecker@nexbridge.com>]

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.