| Message ID | bfe992765cd562b036cb235dfdddb78f5e662812.1725651952.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hash.h: support choosing a separate SHA-1 for non-cryptographic uses | expand |
On Fri, Sep 06, 2024 at 03:46:19PM -0400, Taylor Blau wrote: > Let's work around the issue by using a slightly more modern, but still > quite old v1.6.6.3, which is used by the i0000-basic.sh test script as > well. So I know I shouldn't care that much about which ancient version the interop tests are using. But it feels like we should be able to provide the tools to make this work. How about this instead? -- >8 -- Subject: [PATCH] t/interop: allow per-version make options Building older versions of Git may require tweaking some build knobs. In particular, very old versions of Git will fail to build with recent OpenSSL, because the bignum type switched from a struct to a pointer. The i5500 interop test uses Git v1.0.0 by default, which triggers this problem. You can work around it by setting NO_OPENSSL in your GIT_TEST_MAKE_OPTS variable. But there are two downsides: 1. You have to know to do this, and it's not at all obvious. 2. That sets the options for _all_ versions of Git that we build. And it's possible for two versions to require conflicting knobs. E.g., building with "make NO_OPENSSL=Nope OPENSSL_SHA1=Yes" causes imap-send.c to barf, because it declares a fallback typdef for SSL. This is something we may want to fix, but of course many historical versions are affected, and the interop scripts should be flexible enough to build everything. So let's introduce per-version make options, along with the ability for scripts to specify knobs that match their default versions. That should make everything build out of the box, but also allow testers flexibility if they are testing interoperability between non-default versions. We'll set NO_OPENSSL by default for v1.0.0 in i5500. It doesn't have to worry about the conflict with OPENSSL_SHA1 because imap-send did not exist back then (but if it did, it could also just explicitly use a different hash implementation). Signed-off-by: Jeff King <peff@peff.net> --- t/interop/README | 7 +++++++ t/interop/i5500-git-daemon.sh | 1 + t/interop/interop-lib.sh | 8 +++++--- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/t/interop/README b/t/interop/README index 72d42bd856..4e0608f857 100644 --- a/t/interop/README +++ b/t/interop/README @@ -83,3 +83,10 @@ You can then use test_expect_success as usual, with a few differences: should create one with the appropriate version of git. At the end of the script, call test_done as usual. + +Some older versions may need a few build knobs tweaked (e.g., ancient +versions of Git no longer build with modern OpenSSL). Your script can +set MAKE_OPTS_A and MAKE_OPTS_B, which will be passed alongside +GIT_INTEROP_MAKE_OPTS. Users can override them per-script by setting +GIT_INTEROP_MAKE_OPTS_{A,B} in the environment, just like they can set +GIT_TEST_VERSION_{A,B}. diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh index 4d22e42f84..88712d1b5d 100755 --- a/t/interop/i5500-git-daemon.sh +++ b/t/interop/i5500-git-daemon.sh @@ -2,6 +2,7 @@ VERSION_A=. VERSION_B=v1.0.0 +MAKE_OPTS_B="NO_OPENSSL=TooOld" : ${LIB_GIT_DAEMON_PORT:=5500} LIB_GIT_DAEMON_COMMAND='git.a daemon' diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh index 62f4481b6e..1b5864d2a7 100644 --- a/t/interop/interop-lib.sh +++ b/t/interop/interop-lib.sh @@ -45,7 +45,7 @@ build_version () { ( cd "$dir" && - make $GIT_INTEROP_MAKE_OPTS >&2 && + make $2 $GIT_INTEROP_MAKE_OPTS >&2 && touch .built ) || return 1 @@ -76,9 +76,11 @@ generate_wrappers () { VERSION_A=${GIT_TEST_VERSION_A:-$VERSION_A} VERSION_B=${GIT_TEST_VERSION_B:-$VERSION_B} +MAKE_OPTS_A=${GIT_INTEROP_MAKE_OPTS_A:-$MAKE_OPTS_A} +MAKE_OPTS_B=${GIT_INTEROP_MAKE_OPTS_B:-$MAKE_OPTS_B} -if ! DIR_A=$(build_version "$VERSION_A") || - ! DIR_B=$(build_version "$VERSION_B") +if ! DIR_A=$(build_version "$VERSION_A" "$MAKE_OPTS_A") || + ! DIR_B=$(build_version "$VERSION_B" "$MAKE_OPTS_B") then echo >&2 "fatal: unable to build git versions" exit 1
On Wed, Sep 11, 2024 at 02:10:10AM -0400, Jeff King wrote: > 2. That sets the options for _all_ versions of Git that we build. And > it's possible for two versions to require conflicting knobs. E.g., > building with "make NO_OPENSSL=Nope OPENSSL_SHA1=Yes" causes > imap-send.c to barf, because it declares a fallback typdef for SSL. > This is something we may want to fix, but of course many historical > versions are affected, and the interop scripts should be flexible > enough to build everything. And here's the fix to make this combo work (and likewise, the "fast" variant). We'd still want the interop fix for the reasons given above, but it feels like one less gotcha for people to hit if they are using OPENSSL_SHA1_FAST. -- >8 -- Subject: [PATCH] imap-send: handle NO_OPENSSL even when openssl exists If NO_OPENSSL is defined, then imap-send.c defines a fallback "SSL" type, which is just a void pointer that remains NULL. This works, but it has one problem: it is using the type name "SSL", which conflicts with the upstream name, if some other part of the system happens to include openssl. For example: $ make NO_OPENSSL=Nope OPENSSL_SHA1=Yes imap-send.o CC imap-send.o imap-send.c:35:15: error: conflicting types for ‘SSL’; have ‘void *’ 35 | typedef void *SSL; | ^~~ In file included from /usr/include/openssl/evp.h:26, from sha1/openssl.h:4, from hash.h:10, from object.h:4, from commit.h:4, from refs.h:4, from setup.h:4, from imap-send.c:32: /usr/include/openssl/types.h:187:23: note: previous declaration of ‘SSL’ with type ‘SSL’ {aka ‘struct ssl_st’} 187 | typedef struct ssl_st SSL; | ^~~ make: *** [Makefile:2761: imap-send.o] Error 1 This is not a terribly common combination in practice: 1. Why are we disabling openssl support but still using its sha1? The answer is that you may use the same build options across many versions, and some older versions of Git no longer build with modern versions of openssl. 2. Why are we using a totally unsafe sha1 that does not detect collisions? You're right, we shouldn't. But in preparation for using unsafe sha1 for non-cryptographic checksums, it would be nice to be able to turn it on without hassle. We can make this work by adjusting the way imap-send handles its fallback. One solution is something like this: #ifdef NO_OPENSSL #define git_SSL void * #else #define git_SSL SSL #endif But we can observe that we only need this definition in one spot: the struct which holds the variable. So rather than play around with macros that may cause unexpected effects, we can just directly use the correct type in that struct. Signed-off-by: Jeff King <peff@peff.net> --- imap-send.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/imap-send.c b/imap-send.c index 2dd42807cd..ec68a06687 100644 --- a/imap-send.c +++ b/imap-send.c @@ -31,9 +31,6 @@ #include "parse-options.h" #include "setup.h" #include "strbuf.h" -#if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG) -typedef void *SSL; -#endif #ifdef USE_CURL_FOR_IMAP_SEND #include "http.h" #endif @@ -85,7 +82,11 @@ struct imap_server_conf { struct imap_socket { int fd[2]; +#if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG) + void *ssl; +#else SSL *ssl; +#endif }; struct imap_buffer {
Jeff King <peff@peff.net> writes: > How about this instead? > > -- >8 -- > Subject: [PATCH] t/interop: allow per-version make options > ... > imap-send.c to barf, because it declares a fallback typdef for SSL. "typedef". > diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh > index 4d22e42f84..88712d1b5d 100755 > --- a/t/interop/i5500-git-daemon.sh > +++ b/t/interop/i5500-git-daemon.sh > @@ -2,6 +2,7 @@ > > VERSION_A=. > VERSION_B=v1.0.0 > +MAKE_OPTS_B="NO_OPENSSL=TooOld" > > : ${LIB_GIT_DAEMON_PORT:=5500} > LIB_GIT_DAEMON_COMMAND='git.a daemon' > diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh > index 62f4481b6e..1b5864d2a7 100644 > --- a/t/interop/interop-lib.sh > +++ b/t/interop/interop-lib.sh > @@ -45,7 +45,7 @@ build_version () { > > ( > cd "$dir" && > - make $GIT_INTEROP_MAKE_OPTS >&2 && > + make $2 $GIT_INTEROP_MAKE_OPTS >&2 && The build options should be simple enough and this should do for now (and when it becomes needed, it is easy to add an eval around it). The use of $GIT_INTEROP_MAKE_OPTS here looks a bit curious. It overrides what the inidividual script gave in MAKE_OPTS_{A,B} and what is globally given in GIT_INTEROP_MAKE_OPTS_{A,B}. With this design, the following is not what we should write: # by default we use the frotz feature GIT_INTEROP_MAKE_OPTS=USE_FROTZ=YesPlease # but version A is too old for it MAKE_OPTS_A=USE_FROTZ=NoThanks # we do not need any cutomization for version B MAKE_OPTS_B= Rather we would want to say: # the default should say nothing conflicting with A or B GIT_INTEROP_MAKE_OPTS= # version A is too old to use the frotz feature MAKE_OPTS_A=USE_FROTZ=NoThanks # version B is OK MAKE_OPTS_B=USE_FROTZ=YesPlease As long as it is understood that GIT_INTEROP_MAKE_OPTS and *_{A,B} are *not* meant to be used in a way for one to give default and the other to override the defautl, but they are to give orthogonal settings, this is fine. > touch .built > ) || return 1 > > @@ -76,9 +76,11 @@ generate_wrappers () { > > VERSION_A=${GIT_TEST_VERSION_A:-$VERSION_A} > VERSION_B=${GIT_TEST_VERSION_B:-$VERSION_B} > +MAKE_OPTS_A=${GIT_INTEROP_MAKE_OPTS_A:-$MAKE_OPTS_A} > +MAKE_OPTS_B=${GIT_INTEROP_MAKE_OPTS_B:-$MAKE_OPTS_B} Among the variables we see around here, GIT_INEROP_MAKE_OPTS is the only one that is recorded in the GIT-BUILD-OPTIONS file, which is included in t/interop/interop-lib.sh file. Shouldn't we record GIT_INEROP_MAKE_OPTS_{A,B} as well? > -if ! DIR_A=$(build_version "$VERSION_A") || > - ! DIR_B=$(build_version "$VERSION_B") > +if ! DIR_A=$(build_version "$VERSION_A" "$MAKE_OPTS_A") || > + ! DIR_B=$(build_version "$VERSION_B" "$MAKE_OPTS_B") > then > echo >&2 "fatal: unable to build git versions" > exit 1 Thanks.
On Wed, Sep 11, 2024 at 08:28:37AM -0700, Junio C Hamano wrote: > > - make $GIT_INTEROP_MAKE_OPTS >&2 && > > + make $2 $GIT_INTEROP_MAKE_OPTS >&2 && > > The build options should be simple enough and this should do for now > (and when it becomes needed, it is easy to add an eval around it). > > The use of $GIT_INTEROP_MAKE_OPTS here looks a bit curious. It > overrides what the inidividual script gave in MAKE_OPTS_{A,B} and > what is globally given in GIT_INTEROP_MAKE_OPTS_{A,B}. > > With this design, the following is not what we should write: > > # by default we use the frotz feature > GIT_INTEROP_MAKE_OPTS=USE_FROTZ=YesPlease > # but version A is too old for it > MAKE_OPTS_A=USE_FROTZ=NoThanks > # we do not need any cutomization for version B > MAKE_OPTS_B= > > Rather we would want to say: > > # the default should say nothing conflicting with A or B > GIT_INTEROP_MAKE_OPTS= > # version A is too old to use the frotz feature > MAKE_OPTS_A=USE_FROTZ=NoThanks > # version B is OK > MAKE_OPTS_B=USE_FROTZ=YesPlease > > As long as it is understood that GIT_INTEROP_MAKE_OPTS and *_{A,B} > are *not* meant to be used in a way for one to give default and the > other to override the defautl, but they are to give orthogonal > settings, this is fine. Yes, there are really three levels: what your platform needs for every version, what the script asks about for its specific version, and what you override for that specific version. So arguably the "best" order is: MAKE_OPTS_A < GIT_INTEROP_MAKE_OPTS < GIT_INTEROP_MAKE_OPTS_A which always puts your preferences in front of the script's defaults, but still lets you do a per-script override. But it didn't seem worth the complexity to implement that. I mostly left GIT_INTEROP_MAKE_OPTS_A as an escape hatch if you are testing an alternate version from what's in the script, and I doubt anybody will need it at all (in all these years I have only used it to set NO_OPENSSL for this exact case, and judging by the lack of other people mentioning this issue I suspect hardly anybody else has ever even run these tests). > > @@ -76,9 +76,11 @@ generate_wrappers () { > > > > VERSION_A=${GIT_TEST_VERSION_A:-$VERSION_A} > > VERSION_B=${GIT_TEST_VERSION_B:-$VERSION_B} > > +MAKE_OPTS_A=${GIT_INTEROP_MAKE_OPTS_A:-$MAKE_OPTS_A} > > +MAKE_OPTS_B=${GIT_INTEROP_MAKE_OPTS_B:-$MAKE_OPTS_B} > > Among the variables we see around here, GIT_INEROP_MAKE_OPTS > is the only one that is recorded in the GIT-BUILD-OPTIONS file, > which is included in t/interop/interop-lib.sh file. Shouldn't > we record GIT_INEROP_MAKE_OPTS_{A,B} as well? No, I don't think that would make sense. Everything in GIT-BUILD-OPTIONS, including GIT_INTEROP_MAKE_OPTS, is going to apply to _all_ scripts. These _A and _B variants will vary based on individual scripts. It's possible you might try to run the whole suite between two specific versions, but then you'd set up GIT_INTEROP_MAKE_OPTS_{A,B} in the environment (as you already have to do for VERSION_{A,B}). -Peff
Jeff King <peff@peff.net> writes: > Subject: [PATCH] imap-send: handle NO_OPENSSL even when openssl exists > ... > But we can observe that we only need this definition in one spot: the > struct which holds the variable. So rather than play around with macros > that may cause unexpected effects, we can just directly use the correct > type in that struct. > > Signed-off-by: Jeff King <peff@peff.net> > --- > imap-send.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) Neat. Will queue. Thanks.
diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh index 4d22e42f842..c5bf37e4739 100755 --- a/t/interop/i5500-git-daemon.sh +++ b/t/interop/i5500-git-daemon.sh @@ -1,7 +1,7 @@ #!/bin/sh VERSION_A=. -VERSION_B=v1.0.0 +VERSION_B=v1.6.6.3 : ${LIB_GIT_DAEMON_PORT:=5500} LIB_GIT_DAEMON_COMMAND='git.a daemon'
For the interop tests exercising basic 'git daemon' functionality, we use version v1.0.0 as the old version of Git (which in this test we happen to designate with using VERSION_B). But that version does not compile with modern versions of OpenSSL, complaining with error messages like: epoch.c:21:16: error: field ‘numerator’ has incomplete type 21 | BIGNUM numerator; | ^~~~~~~~~ epoch.c:22:16: error: field ‘denominator’ has incomplete type 22 | BIGNUM denominator; | ^~~~~~~~~~~ epoch.c: In function ‘new_zero’: Of course, compiling with `NO_OPENSSL=1`, which we have had since dd53c7ab297 ([PATCH] Support for NO_OPENSSL, 2005-07-29) allows us to compile cleanly. This hasn't been such a problem in practice because most builds can use NO_OPENSSL when compiling the older versions of Git used by the interop tests, because often even the current version of Git does not use OpenSSL (e.g., because we use the collision detecting implementation of SHA-1). But subsequent changes will make a build configuration that does use OpenSSL's SHA-1 implementation (at least for non-cryptographic uses) more common, thus breaking this interop build (since only one side will compile with NO_OPENSSL). Let's work around the issue by using a slightly more modern, but still quite old v1.6.6.3, which is used by the i0000-basic.sh test script as well. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/interop/i5500-git-daemon.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)