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

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

[Randall S. Becker]

This simple series adds the zlib and libcurl versions, if any, used during a git
build.

As an example, the following is appended to the git version --build-options
report:

        libcurl: 8.7.1
	zlib: 1.3.1

Randall S. Becker (1):
  Teach git version --build-options to know about zlib and libcurl
    versions.

 help.c | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.43.0

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

[Randall S. Becker]

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(+)

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
 	}
 }
 
-- 
2.43.0

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

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

[rsbecker]

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.

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

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

[Randall Becker]

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

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

[Junio C Hamano]

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

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

[rsbecker]

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

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

[Junio C Hamano]

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

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

[rsbecker]

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.

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

[Jeff King]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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.