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

From: rsbecker@nexbridge.com <rsbecker@nexbridge.com>
Date: Jun 21, 2024, 18:33 UTC
Message-ID: <016501dac409$7dd5bc00$79813400$@nexbridge.com>
In-Reply-To: <xmqqmsnekvir.fsf@gitster.g>

On Friday, June 21, 2024 2:20 PM, Junio C Hamano wrote:

Show 8 quoted lines

>"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.

Show 8 quoted lines

>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.

Show 14 quoted lines

>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

Show 14 quoted lines

>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

Show 12 quoted lines

>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.