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
