Re: [PATCH v3 5/9] i5500-git-daemon.sh: use compile-able version of Git without OpenSSL

From: Jeff King <peff@peff.net>
Date: Sep 11, 2024, 21:23 UTC
Message-ID: <20240911212359.GA1659@coredump.intra.peff.net>
In-Reply-To: <xmqqr09qb462.fsf@gitster.g>

On Wed, Sep 11, 2024 at 08:28:37AM -0700, Junio C Hamano wrote:

Show 32 quoted lines

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

Show 11 quoted lines

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