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

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?

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)

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.

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.

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.

-Peff