[PATCH v0 0/1] Teach git version --build-options about OpenSSL

[PATCH v0 0/1] Teach git version --build-options about OpenSSL

[Randall S. Becker]

This simple series adds the OpenSSL version, if any, used during a git
build.

As an example, if built with OpenSSL 3.0, the following is appended to
the git version --build-options report:

	OpenSSL: OpenSSL 3.0.13 30 Jan 2024

Randall S. Becker (1):
  Teach git version --build-options to know about the OpenSSL version
    used.

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

-- 
2.43.0

[PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Randall S. Becker]

This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define supplied
for this purpose by that project. If the #define is not present, the version
is not reported.

Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
 help.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/help.c b/help.c
index 1d057aa607..ce55aaa2c0 100644
--- a/help.c
+++ b/help.c
@@ -757,6 +757,9 @@ 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 OPENSSL_VERSION_TEXT
+		strbuf_addf(buf, "OpenSSL: %s\n", OPENSSL_VERSION_TEXT);
+#endif
 	}
 }
 
-- 
2.43.0

Re: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Junio C Hamano]

"Randall S. Becker" <the.n.e.key@gmail.com> writes:

> This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define supplied
> for this purpose by that project. If the #define is not present, the version
> is not reported.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  help.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/help.c b/help.c
> index 1d057aa607..ce55aaa2c0 100644
> --- a/help.c
> +++ b/help.c
> @@ -757,6 +757,9 @@ 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 OPENSSL_VERSION_TEXT
> +		strbuf_addf(buf, "OpenSSL: %s\n", OPENSSL_VERSION_TEXT);
> +#endif
>  	}
>  }

It is kind-a surprising that we do not need to play with any
Makefile macros for this implementation.

If some unknown version (either in the long past or in the future)
of OpenSSL does not define the constant, this is just compiled out
and that would be OK.

If some unknown version of OpenSSL does define it but not as a
string constant, it would break the build, e.g.,

	#define OPENSSL_VERSION_TEXT 2 plus 4 is 6

We could stringify it ourselves, but that is probably not worth
worrying about.

Will queue.  Thanks.

Re: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define supplied
>> for this purpose by that project. If the #define is not present, the version
>> is not reported.
> ...
> If some unknown version of OpenSSL does define it but not as a
> string constant, it would break the build, e.g.,
>
> 	#define OPENSSL_VERSION_TEXT 2 plus 4 is 6
>
> We could stringify it ourselves, but that is probably not worth
> worrying about.
>
> Will queue.  Thanks.

Having said that, we do link with and depend on libraries like
libcURL, libPCRE, libz, etc.  I wonder if they are also worth
reporting, and if so how?

We can leave it just like any other new features, "if you have an
itch to see it, you can offer a patch", but I am wondering if we are
going to get a several more, we'd at least want to standardize the
process and the output (e.g., do we limit the line counts to 1 and
line length to some reasonably low number?).

Thanks.

RE: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Randall Becker]

On Thursday, June 20, 2024 2:35 PM, Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>
>> "Randall S. Becker" <the.n.e.key@gmail.com> writes:
>>
>>> This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define
>>> supplied for this purpose by that project. If the #define is not
>>> present, the version is not reported.
>> ...
>> If some unknown version of OpenSSL does define it but not as a string
>> constant, it would break the build, e.g.,
>>
>> 	#define OPENSSL_VERSION_TEXT 2 plus 4 is 6
>>
>> We could stringify it ourselves, but that is probably not worth
>> worrying about.
>>
>> Will queue.  Thanks.
>
>Having said that, we do link with and depend on libraries like libcURL, libPCRE, libz,
>etc.  I wonder if they are also worth reporting, and if so how?
>
>We can leave it just like any other new features, "if you have an itch to see it, you
>can offer a patch", but I am wondering if we are going to get a several more, we'd at
>least want to standardize the process and the output (e.g., do we limit the line
>counts to 1 and line length to some reasonably low number?).

I was thinking about those also, but this was a minimalist implementation (opensslv.h comes in via git-compat-util.h - so I did not need to bring anything else into the code). If anyone is interested, the libcurl version is in curlver.h (LIBCURL_VERSION) - but I think it is more important to have the OpenSSL version as this is an important compatibility indicator. zlib.h has ZLIB_VERSION (text form). I don't have libPCRE, so can't tell. I can do another series for the others, but those would impact help.c includes, unlike this one.

RE: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[rsbecker]

On Thursday, June 20, 2024 2:48 PM, Randall Becker wrote:
>On Thursday, June 20, 2024 2:35 PM, Junio C Hamano wrote:
>>Junio C Hamano <gitster@pobox.com> writes:
>>
>>> "Randall S. Becker" <the.n.e.key@gmail.com> writes:
>>>
>>>> This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define
>>>> supplied for this purpose by that project. If the #define is not
>>>> present, the version is not reported.
>>> ...
>>> If some unknown version of OpenSSL does define it but not as a string
>>> constant, it would break the build, e.g.,
>>>
>>> 	#define OPENSSL_VERSION_TEXT 2 plus 4 is 6
>>>
>>> We could stringify it ourselves, but that is probably not worth
>>> worrying about.
>>>
>>> Will queue.  Thanks.
>>
>>Having said that, we do link with and depend on libraries like libcURL,
>>libPCRE, libz, etc.  I wonder if they are also worth reporting, and if so
how?
>>
>>We can leave it just like any other new features, "if you have an itch
>>to see it, you can offer a patch", but I am wondering if we are going
>>to get a several more, we'd at least want to standardize the process
>>and the output (e.g., do we limit the line counts to 1 and line length to
some
>reasonably low number?).
>
>I was thinking about those also, but this was a minimalist implementation
>(opensslv.h comes in via git-compat-util.h - so I did not need to bring
anything else
>into the code). If anyone is interested, the libcurl version is in
curlver.h
>(LIBCURL_VERSION) - but I think it is more important to have the OpenSSL
version
>as this is an important compatibility indicator. zlib.h has ZLIB_VERSION
(text form). I
>don't have libPCRE, so can't tell. I can do another series for the others,
but those
>would impact help.c includes, unlike this one.

I have another patch almost ready for zlib and libcurl, but it is based on
the OpenSSL change. Would you like a re-roll or should I wait for the merge?
I do not have the PCRE - not available on my system, so someone else should
do that one.

Re: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> I have another patch almost ready for zlib and libcurl, but it is based on
> the OpenSSL change. Would you like a re-roll or should I wait for the merge?
> I do not have the PCRE - not available on my system, so someone else should
> do that one.

A two-patch series for zlib and libcURL that builds on top of
8b731b8d (version: --build-options reports OpenSSL version
information, 2024-06-19), which has already hit 'next', would be OK,
but most likely, these three are independent "for X in (cURL, zlib,
OpenSSL), append X if X is there", and when the three changes are
merged together, it would result in

    #if defined CURL_something
	strbuf_add*(...libcurl thing...);
    #endif
    #if defined OPENSSL_something
	strbuf_add*(...openssl thing...);
    #endif
    #if defined libz_something
	strbuf_add*(...zlib thing...);
    #endif

with possible permutation of different ordering of them.  And in
such a case, three parallel topics that build on the same base
(i.e. some recent tip of 'master') would be just fine, even though
they _surely_ will introduce trivial textual conflicts.

If you introduced a helper function or CPP macro to make it easy to
add the OpenSSL version string in your OpenSSL patch, and the other
two patches took advantage of the helper or CPP macro while adding
the zlib or libcURL version string, then it would be a different
story.  A two-patch series for zlib and libcURL that builds on top
of the OpenSSL patch would become the best (and the only practical)
approach in such a case, but there is nothing in the OpenSSL patch
we have reviewed that these other two would want to depend on, so...

RE: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[rsbecker]

On Thursday, June 20, 2024 7:55 PM, Junio C Hamano wrote:
><rsbecker@nexbridge.com> writes:
>
>> I have another patch almost ready for zlib and libcurl, but it is
>> based on the OpenSSL change. Would you like a re-roll or should I wait
for the
>merge?
>> I do not have the PCRE - not available on my system, so someone else
>> should do that one.
>
>A two-patch series for zlib and libcURL that builds on top of 8b731b8d
(version: --
>build-options reports OpenSSL version information, 2024-06-19), which has
>already hit 'next', would be OK, but most likely, these three are
independent "for X
>in (cURL, zlib, OpenSSL), append X if X is there", and when the three
changes are
>merged together, it would result in
>
>    #if defined CURL_something
>	strbuf_add*(...libcurl thing...);
>    #endif
>    #if defined OPENSSL_something
>	strbuf_add*(...openssl thing...);
>    #endif
>    #if defined libz_something
>	strbuf_add*(...zlib thing...);
>    #endif
>
>with possible permutation of different ordering of them.  And in such a
case, three
>parallel topics that build on the same base (i.e. some recent tip of
'master') would
>be just fine, even though they _surely_ will introduce trivial textual
conflicts.
>
>If you introduced a helper function or CPP macro to make it easy to add the
>OpenSSL version string in your OpenSSL patch, and the other two patches
took
>advantage of the helper or CPP macro while adding the zlib or libcURL
version
>string, then it would be a different story.  A two-patch series for zlib
and libcURL that
>builds on top of the OpenSSL patch would become the best (and the only
practical)
>approach in such a case, but there is nothing in the OpenSSL patch we have
>reviewed that these other two would want to depend on, so...

I think I would rather let each one stand. Embedding an #if defined inside a
macro makes me nervous, considering it is compiler version dependent. Would
putting each one in its own commit work for you?
--Randall

Re: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Junio C Hamano]

<rsbecker@nexbridge.com> writes:

> I think I would rather let each one stand.

Yup.  Two patches (one for zlib, another for libcURL) next to the
one already done for OpenSSL, would be good.

RE: [PATCH v0 1/1] Teach git version --build-options about OpenSSL

[Randall Becker]

On Thursday, June 20, 2024 2:27 PM, Junio C Hamano wrote:
>"Randall S. Becker" <the.n.e.key@gmail.com> writes:
>
>> This change uses the OpenSSL supplied OPENSSL_VERSION_TEXT #define
>> supplied for this purpose by that project. If the #define is not
>> present, the version is not reported.
>>
>> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> ---
>>  help.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/help.c b/help.c
>> index 1d057aa607..ce55aaa2c0 100644
>> --- a/help.c
>> +++ b/help.c
>> @@ -757,6 +757,9 @@ 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 OPENSSL_VERSION_TEXT
>> +		strbuf_addf(buf, "OpenSSL: %s\n", OPENSSL_VERSION_TEXT);
>#endif
>>  	}
>>  }
>
>It is kind-a surprising that we do not need to play with any Makefile macros for this
>implementation.
>
>If some unknown version (either in the long past or in the future) of OpenSSL does
>not define the constant, this is just compiled out and that would be OK.
>
>If some unknown version of OpenSSL does define it but not as a string constant, it
>would break the build, e.g.,
>
>	#define OPENSSL_VERSION_TEXT 2 plus 4 is 6
>
>We could stringify it ourselves, but that is probably not worth worrying about.

AFAIK, this #define is guaranteed to be TEXT by OpenSSL. There are other forms of the version content that are numeric. I think this was a change introduced around v1.1.1. Now that opensslv.h is included, we can depend on this being available (or not if it is not defined). Stringification should not be required. It is present through the upcoming v3.4. There are other string and numeric forms of the version, but this one is most informative (tag and date) from a support standpoint. The only thing missing is the git commit, but it would take a lot to get that put into that project.

>Will queue.  Thanks.

Thanks.