| Message ID | Z8IX2bMJe+V80idE@nand.local (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | GIT-BUILD-OPTIONS can override manual invocations | expand |
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
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
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
--- a/Makefile +++ b/Makefile @@ -3145,6 +3145,7 @@ endif # and the first level quoting from the shell that runs "echo". GIT-BUILD-OPTIONS: FORCE @sed \ + $(if $(GIT_PERF_REPO),, -e "/^GIT_PERF_REPO=/d") \ -e "s!@BROKEN_PATH_FIX@!\'$(BROKEN_PATH_FIX)\'!" \ -e "s|@DIFF@|\'$(DIFF)\'|" \ -e "s|@FSMONITOR_DAEMON_BACKEND@|\'$(FSMONITOR_DAEMON_BACKEND)\'|" \ --- >8 ---
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! For example, I removed the libexpat headers from my system, and ran the above to get the following output: $ find /usr/include -name expat.h | wc -l 0 $ make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1' [...] http-push.c:28:10: fatal error: expat.h: No such file or directory 28 | #include <expat.h> | ^~~~~~~~~ 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. Interestingly, 4638e8806e works around a similar issue in test-lib.sh where it stores the value of $TEST_OUTPUT_DIRECTORY in a temporary variable, and restores it after sourcing GIT-BUILD-OPTIONS if $TEST_OUTPUT_DIRECTORY is still empty. I think even this is subtly broken if your $TEST_OUTPUT_DIRECTORY is set to different non-empty values between GIT-BUILD-OPTIONS and when test-lib.sh is executed. I noticed this along with Elijah while merging v2.48.1 into GitHub's private fork since our CI suite runs the t/interop tests with a custom GIT_INTEROP_MAKE_OPTS. We could partially fix this in the same way as we do for TEST_OUTPUT_DIRECTORY, but I think that this isn't quite correct, and it makes me uneasy knowing that there are other places we might face similar issues. AFAICT, 4638e8806e has the potential to disrupt scripts that use any of the following variables: - FSMONITOR_DAEMON_BACKEND - FSMONITOR_OS_SETTINGS - GIT_INTEROP_MAKE_OPTS - GIT_PERF_LARGE_REPO - GIT_PERF_MAKE_COMMAND - GIT_PERF_MAKE_OPTS - GIT_PERF_REPEAT_COUNT - GIT_PERF_REPO - GIT_TEST_CMP - GIT_TEST_CMP_USE_COPIED_CONTEXT - GIT_TEST_INDEX_VERSION - GIT_TEST_OPTS - GIT_TEST_PERL_FATAL_WARNINGS - GIT_TEST_UTF8_LOCALE - TEST_OUTPUT_DIRECTORY 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: --- 8< --- diff --git a/Makefile b/Makefile index 97e8385b66..35e5571d8e 100644 , and similar could work for the Makefile, but I'm not sure what the Meson equivalent would be, or if this is even a good idea or not. Thoughts? Thanks, Taylor