On Tue, Mar 04, 2025 at 05:08:59PM -0500, Taylor Blau wrote:
> > 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='...'
Ah, that makes much more sense.
And while it's the same issue as my:
> > make &&
> > (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)
I can see how it's even more confusing, since you are feeding it as a
make variable, and not through the environment (though of course it is
through the environment that you'd eventually expect it to make it).
> > 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.
IMHO the root of the problem is writing GIT_PERF_MAKE_OPTS and
GIT_INTEROP_MAKE_OPTS into GIT-BUILD-OPTIONS at all. It is not used in
the actual build of Git run by the initial "make" invocation, and if it
were to change we would not actually need to rebuild anything. And yet:
make GIT_PERF_MAKE_OPTS=foo
will not fail (since we don't look at the nonsense value), and:
make GIT_PERF_MAKE_OPTS=bar
will rebuild at least scripts because it doesn't know that the changed
value is uninteresting.
If we instead just read the value when running "make -C t/interop", we'd
always get the fresh value (whether from the command-line, the
environment, or reading config.mak).
But there are two catches:
- right now if you do:
make GIT_INTEROP_MAKE_OPTS=whatever
make -C t/interop
it will use the value from the original "make". I think that is the
root of the confusion, but possibly people depend on that? I don't
know. I would (and in fact do) put it into my config.mak, where it
would be seen by both invocations.
- we'd want to see the values from both make and the shell, and their
syntaxes are slightly different. In particular, right now I'd do:
echo GIT_PERF_MAKE_OPTS=whatever >>config.mak
make ;# this writes it into GIT-BUILD-OPTIONS
cd t/perf
./run HEAD^ HEAD ;# this sources GIT-BUILD-OPTIONS
How does that work without GIT-BUILD-OPTIONS? I'd need ./run to
source the config.mak file, but it's in make format, not shell. So
I'd have to do something like this in the run script:
opts=$(make -epn | perl -lne 'print $1 if /^GIT_PERF_MAKE_OPTS = (.*)/')
(or there are variants that can be implemented with a special
target). It's an extra step, but maybe not too bad.
So I dunno. It may not be worth changing the status quo.
> > > 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?
I think you were lucky that giving a different GIT_PERF_MAKE_OPTS
variable to your second make invocation ever worked at all. It was not
not the intended mechanism (at least that is my understanding; like I
said, most of this was not documented and the implications of various
approaches were probably not through through all that much).
So yes, 4638e8806e is a regression for your particular invocation, but
given the adjacent issues (if you had those variables set in your
config.mak), is it worth trying to support your case? Or put another
way, is there any reason you can't just do:
make GIT_INTEROP_MAKE_OPTS=whatever && make -C t/interop
? If this change impacted a lot of users, I'd be more worried about
retaining corner cases. But the perf and interop suites are developer
tools, and I'd guess that only a tiny fraction of Git devs even use
them.
Not that I am particularly opposed to your "don't write empty values to
the GIT-BUILD-OPTIONS" solution. My main complaint is just that it's new
extra code, though if we can at least write it _once_ instead of having
to remember to do it for each variable, that's not too bad.
-Peff
