Re: GIT-BUILD-OPTIONS can override manual invocations

[Taylor Blau <me@ttaylorr.com>]

On Tue, Mar 04, 2025 at 03:29:01AM -0500, Jeff King wrote:
> On Fri, Feb 28, 2025 at 03:08:57PM -0500, Taylor Blau wrote:
>
> > In 4638e8806e (Makefile: use common template for GIT-BUILD-OPTIONS,
> > 2024-12-06), the project's Makefile changed how it writes the
> > GIT-BUILD-OPTIONS script. Prior to 4638e8806e, the Makefile would write
> > the file itself, but post-4638e8806e it fills out a template
> > ("GIT-BUILD-OPTIONS.in") with the appropriate values.
> >
> > This has an interesting side effect when running e.g. the t/perf or
> > t/interop suites. If I do:
> >
> >     make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1'
> >
> > , then we will still try and build with the libexpat headers!
>
> Hmm. I am not sure what this is supposed to do, as I would not expect
> that "make -C t/perf" to build anything at all. It will use the working
> tree version built in the first step. So I'd expect your initial "make"
> to do all the work (and either fail or not depending on whether NO_EXPAT
> is set in your config.mak).

Oops, I should have used the t/interop example instead of t/perf from
above. I agree that t/perf doesn't build anything. The buggy invocation
that Elijah and I noticed was:

    make &&
    make -C t/interop GIT_INTEROP_MAKE_OPTS='...'

> I usually trigger a build of another version using arguments to "./run".
> Is there a way to make that happen via make in t/perf?

I don't think there is via "make" (at least, I couldn't think of one off
the top of my head), but I imagine if you set e.g., GIT_PERF_REPO in
your environment when calling t/perf/run (and didn't have it set when
you originally built with 'make') that you'd run into similar issues.

> > This is AFAICT fallout from a change in 4638e8806e where instead of
> > *not* writing e.g. GIT_PERF_MAKE_OPTS into the GIT-BUILD-OPTIONS file,
> > we now write it with an empty value. So when we run 'make -C t/perf'
> > with a non-empty GIT_PERF_MAKE_OPTS, t/perf/run will source
> > GIT-BUILD-OPTIONS, and override the value of GIT_PERF_MAKE_OPTS we
> > specified.
>
> But yeah, I can see how this would fail with:
>
>   make &&
>   (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)

Exactly.

> if the GIT-BUILD-OPTIONS value takes precedence over the environment.
> OTOH, wasn't that also true before 4638e8806e if you did set
> GIT_PERF_MAKE_OPTS? So:
>
>   make GIT_PERF_MAKE_OPTS=NO_TCLTK=1 &&
>   (cd t/perf && GIT_PERF_MAKE_OPTS="NO_TCLTK=1 NO_EXPAT=1" ./run HEAD^ HEAD)
>
> would fail (or more likely, the initial one is set in your config.mak).
>
> I think you're "supposed" to do this:
>
>   make GIT_PERF_MAKE_OPTS=NO_EXPAT=1 &&
>   (cd t/perf && ./run HEAD^ HEAD)
>
> Rather than rely on the environment. But of course none of that is
> documented at all, and is just convention and the whims of the few
> people who bothered to run t/perf at all in the first place.

Thanks for pointing out the subtlety there. I agree that pre-4638e8806e
it was a bug to do

    make GIT_PERF_MAKE_OPTS=ABC &&
    (cd t/perf && GIT_PERF_MAKE_OPTS="ABC XYZ" ./run HEAD^ HEAD)

since the GIT-BUILD-OPTIONS values override the environment variables
when the ./run script is ran.

But it feels like a regression that in addition to the above now:

    make &&
    (cd t/perf && GIT_PERF_MAKE_OPTS=ABC ./run HEAD^ HEAD)

is broken, too, since GIT_PERF_MAKE_OPTS wasn't set in the original make
invocation at all!

> I do think it would be nice if environment variables took precedence
> over the sourced GIT-BUILD-OPTIONS for "./run", but I suspect doing so
> is a little tricky.

Yeah, I agree, and I think that would be tantamount to also fixing the
pre-4638e8806e behavior, which would be nice. I think a good middle
ground would be to continue to allow environment variables to override
options that are unset in GIT-BUILD-OPTIONS, which definitely is a
regression in 4638e8806e.

> > So I think a more robust fix might look like only filling out those
> > lines in the GIT-BUILD-OPTIONS template when they are non-empty, similar
> > to the pre-4638e8806e behavior. Something like:
>
> Yeah, that would fix the regression. But I kind of feel like your
> initial command is already skirting the edges of what the original code
> was meant to handle.

Hmm. I'm not sure I am following what you're saying here. How so?

Thanks,
Taylor