| Message ID | 20240619172421.33548-2-randall.becker@nexbridge.ca (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8b731b8d06b710ce25dcef8134db30e1389dd92f |
| Headers | show |
| Series | Teach git version --build-options about OpenSSL | expand |
"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.
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.
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.
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.
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.
<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...
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
<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.
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 } }
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(+)