{"thread":{"id":"63026","subject":"GIT-BUILD-OPTIONS can override manual invocations","startedAt":"2025-02-28T20:09:00Z","lastAt":"2025-03-05T07:57:18Z","messageCount":4,"participants":["Taylor Blau","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"513295","messageId":"Z8IX2bMJe+V80idE@nand.local","threadId":"63026","inReplyTo":null,"subject":"GIT-BUILD-OPTIONS can override manual invocations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T20:08:57Z","receivedAt":"2025-02-28T20:09:00Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 4638e8806e (Makefile: use common template for GIT-BUILD-OPTIONS,\n2024-12-06), the project's Makefile changed how it writes the\nGIT-BUILD-OPTIONS script. Prior to 4638e8806e, the Makefile would write\nthe file itself, but post-4638e8806e it fills out a template\n(\"GIT-BUILD-OPTIONS.in\") with the appropriate values.\n\nThis has an interesting side effect when running e.g. the t/perf or\nt/interop suites. If I do:\n\n    make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1'\n\n, then we will still try and build with the libexpat headers!\n\nFor example, I removed the libexpat headers from my system, and ran the\nabove to get the following output:\n\n    $ find /usr/include -name expat.h | wc -l\n    0\n\n    $ make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1'\n    [...]\n    http-push.c:28:10: fatal error: expat.h: No such file or directory\n       28 | #include <expat.h>\n          |          ^~~~~~~~~\n\nThis is AFAICT fallout from a change in 4638e8806e where instead of\n*not* writing e.g. GIT_PERF_MAKE_OPTS into the GIT-BUILD-OPTIONS file,\nwe now write it with an empty value. So when we run 'make -C t/perf'\nwith a non-empty GIT_PERF_MAKE_OPTS, t/perf/run will source\nGIT-BUILD-OPTIONS, and override the value of GIT_PERF_MAKE_OPTS we\nspecified.\n\nInterestingly, 4638e8806e works around a similar issue in test-lib.sh\nwhere it stores the value of $TEST_OUTPUT_DIRECTORY in a temporary\nvariable, and restores it after sourcing GIT-BUILD-OPTIONS if\n$TEST_OUTPUT_DIRECTORY is still empty. I think even this is subtly\nbroken if your $TEST_OUTPUT_DIRECTORY is set to different non-empty\nvalues between GIT-BUILD-OPTIONS and when test-lib.sh is executed.\n\nI noticed this along with Elijah while merging v2.48.1 into GitHub's\nprivate fork since our CI suite runs the t/interop tests with a custom\nGIT_INTEROP_MAKE_OPTS.\n\nWe could partially fix this in the same way as we do for\nTEST_OUTPUT_DIRECTORY, but I think that this isn't quite correct, and it\nmakes me uneasy knowing that there are other places we might face\nsimilar issues. AFAICT, 4638e8806e has the potential to disrupt scripts\nthat use any of the following variables:\n\n  - FSMONITOR_DAEMON_BACKEND\n  - FSMONITOR_OS_SETTINGS\n  - GIT_INTEROP_MAKE_OPTS\n  - GIT_PERF_LARGE_REPO\n  - GIT_PERF_MAKE_COMMAND\n  - GIT_PERF_MAKE_OPTS\n  - GIT_PERF_REPEAT_COUNT\n  - GIT_PERF_REPO\n  - GIT_TEST_CMP\n  - GIT_TEST_CMP_USE_COPIED_CONTEXT\n  - GIT_TEST_INDEX_VERSION\n  - GIT_TEST_OPTS\n  - GIT_TEST_PERL_FATAL_WARNINGS\n  - GIT_TEST_UTF8_LOCALE\n  - TEST_OUTPUT_DIRECTORY\n\nSo I think a more robust fix might look like only filling out those\nlines in the GIT-BUILD-OPTIONS template when they are non-empty, similar\nto the pre-4638e8806e behavior. Something like:\n\n--- 8< ---\ndiff --git a/Makefile b/Makefile\nindex 97e8385b66..35e5571d8e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3145,6 +3145,7 @@ endif\n # and the first level quoting from the shell that runs \"echo\".\n GIT-BUILD-OPTIONS: FORCE\n \t@sed \\\n+\t\t$(if $(GIT_PERF_REPO),, -e \"/^GIT_PERF_REPO=/d\") \\\n \t\t-e \"s!@BROKEN_PATH_FIX@!\\'$(BROKEN_PATH_FIX)\\'!\" \\\n \t\t-e \"s|@DIFF@|\\'$(DIFF)\\'|\" \\\n \t\t-e \"s|@FSMONITOR_DAEMON_BACKEND@|\\'$(FSMONITOR_DAEMON_BACKEND)\\'|\" \\\n--- >8 ---\n\n, and similar could work for the Makefile, but I'm not sure what\nthe Meson equivalent would be, or if this is even a good idea or not.\n\nThoughts?\n\nThanks,\nTaylor\n"},{"id":"513506","messageId":"20250304082901.GA1297837@coredump.intra.peff.net","threadId":"63026","inReplyTo":"Z8IX2bMJe+V80idE@nand.local","subject":"Re: GIT-BUILD-OPTIONS can override manual invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T08:29:01Z","receivedAt":"2025-03-04T08:29:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2025 at 03:08:57PM -0500, Taylor Blau wrote:\n\n> In 4638e8806e (Makefile: use common template for GIT-BUILD-OPTIONS,\n> 2024-12-06), the project's Makefile changed how it writes the\n> GIT-BUILD-OPTIONS script. Prior to 4638e8806e, the Makefile would write\n> the file itself, but post-4638e8806e it fills out a template\n> (\"GIT-BUILD-OPTIONS.in\") with the appropriate values.\n> \n> This has an interesting side effect when running e.g. the t/perf or\n> t/interop suites. If I do:\n> \n>     make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1'\n> \n> , then we will still try and build with the libexpat headers!\n\nHmm. I am not sure what this is supposed to do, as I would not expect\nthat \"make -C t/perf\" to build anything at all. It will use the working\ntree version built in the first step. So I'd expect your initial \"make\"\nto do all the work (and either fail or not depending on whether NO_EXPAT\nis set in your config.mak).\n\nI usually trigger a build of another version using arguments to \"./run\".\nIs there a way to make that happen via make in t/perf?\n\n> This is AFAICT fallout from a change in 4638e8806e where instead of\n> *not* writing e.g. GIT_PERF_MAKE_OPTS into the GIT-BUILD-OPTIONS file,\n> we now write it with an empty value. So when we run 'make -C t/perf'\n> with a non-empty GIT_PERF_MAKE_OPTS, t/perf/run will source\n> GIT-BUILD-OPTIONS, and override the value of GIT_PERF_MAKE_OPTS we\n> specified.\n\nBut yeah, I can see how this would fail with:\n\n  make &&\n  (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)\n\nif the GIT-BUILD-OPTIONS value takes precedence over the environment.\nOTOH, wasn't that also true before 4638e8806e if you did set\nGIT_PERF_MAKE_OPTS? So:\n\n  make GIT_PERF_MAKE_OPTS=NO_TCLTK=1 &&\n  (cd t/perf && GIT_PERF_MAKE_OPTS=\"NO_TCLTK=1 NO_EXPAT=1\" ./run HEAD^ HEAD)\n\nwould fail (or more likely, the initial one is set in your config.mak).\n\nI think you're \"supposed\" to do this:\n\n  make GIT_PERF_MAKE_OPTS=NO_EXPAT=1 &&\n  (cd t/perf && ./run HEAD^ HEAD)\n\nRather than rely on the environment. But of course none of that is\ndocumented at all, and is just convention and the whims of the few\npeople who bothered to run t/perf at all in the first place.\n\nI do think it would be nice if environment variables took precedence\nover the sourced GIT-BUILD-OPTIONS for \"./run\", but I suspect doing so\nis a little tricky.\n\n> So I think a more robust fix might look like only filling out those\n> lines in the GIT-BUILD-OPTIONS template when they are non-empty, similar\n> to the pre-4638e8806e behavior. Something like:\n\nYeah, that would fix the regression. But I kind of feel like your\ninitial command is already skirting the edges of what the original code\nwas meant to handle.\n\n-Peff\n"},{"id":"513578","messageId":"Z8d5++1dNdo/32uz@nand.local","threadId":"63026","inReplyTo":"20250304082901.GA1297837@coredump.intra.peff.net","subject":"Re: GIT-BUILD-OPTIONS can override manual invocations","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-04T22:08:59Z","receivedAt":"2025-03-04T22:09:14Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 04, 2025 at 03:29:01AM -0500, Jeff King wrote:\n> On Fri, Feb 28, 2025 at 03:08:57PM -0500, Taylor Blau wrote:\n>\n> > In 4638e8806e (Makefile: use common template for GIT-BUILD-OPTIONS,\n> > 2024-12-06), the project's Makefile changed how it writes the\n> > GIT-BUILD-OPTIONS script. Prior to 4638e8806e, the Makefile would write\n> > the file itself, but post-4638e8806e it fills out a template\n> > (\"GIT-BUILD-OPTIONS.in\") with the appropriate values.\n> >\n> > This has an interesting side effect when running e.g. the t/perf or\n> > t/interop suites. If I do:\n> >\n> >     make && make -C t/perf GIT_PERF_MAKE_OPTS='NO_EXPAT=1'\n> >\n> > , then we will still try and build with the libexpat headers!\n>\n> Hmm. I am not sure what this is supposed to do, as I would not expect\n> that \"make -C t/perf\" to build anything at all. It will use the working\n> tree version built in the first step. So I'd expect your initial \"make\"\n> to do all the work (and either fail or not depending on whether NO_EXPAT\n> is set in your config.mak).\n\nOops, I should have used the t/interop example instead of t/perf from\nabove. I agree that t/perf doesn't build anything. The buggy invocation\nthat Elijah and I noticed was:\n\n    make &&\n    make -C t/interop GIT_INTEROP_MAKE_OPTS='...'\n\n> I usually trigger a build of another version using arguments to \"./run\".\n> Is there a way to make that happen via make in t/perf?\n\nI don't think there is via \"make\" (at least, I couldn't think of one off\nthe top of my head), but I imagine if you set e.g., GIT_PERF_REPO in\nyour environment when calling t/perf/run (and didn't have it set when\nyou originally built with 'make') that you'd run into similar issues.\n\n> > This is AFAICT fallout from a change in 4638e8806e where instead of\n> > *not* writing e.g. GIT_PERF_MAKE_OPTS into the GIT-BUILD-OPTIONS file,\n> > we now write it with an empty value. So when we run 'make -C t/perf'\n> > with a non-empty GIT_PERF_MAKE_OPTS, t/perf/run will source\n> > GIT-BUILD-OPTIONS, and override the value of GIT_PERF_MAKE_OPTS we\n> > specified.\n>\n> But yeah, I can see how this would fail with:\n>\n>   make &&\n>   (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)\n\nExactly.\n\n> if the GIT-BUILD-OPTIONS value takes precedence over the environment.\n> OTOH, wasn't that also true before 4638e8806e if you did set\n> GIT_PERF_MAKE_OPTS? So:\n>\n>   make GIT_PERF_MAKE_OPTS=NO_TCLTK=1 &&\n>   (cd t/perf && GIT_PERF_MAKE_OPTS=\"NO_TCLTK=1 NO_EXPAT=1\" ./run HEAD^ HEAD)\n>\n> would fail (or more likely, the initial one is set in your config.mak).\n>\n> I think you're \"supposed\" to do this:\n>\n>   make GIT_PERF_MAKE_OPTS=NO_EXPAT=1 &&\n>   (cd t/perf && ./run HEAD^ HEAD)\n>\n> Rather than rely on the environment. But of course none of that is\n> documented at all, and is just convention and the whims of the few\n> people who bothered to run t/perf at all in the first place.\n\nThanks for pointing out the subtlety there. I agree that pre-4638e8806e\nit was a bug to do\n\n    make GIT_PERF_MAKE_OPTS=ABC &&\n    (cd t/perf && GIT_PERF_MAKE_OPTS=\"ABC XYZ\" ./run HEAD^ HEAD)\n\nsince the GIT-BUILD-OPTIONS values override the environment variables\nwhen the ./run script is ran.\n\nBut it feels like a regression that in addition to the above now:\n\n    make &&\n    (cd t/perf && GIT_PERF_MAKE_OPTS=ABC ./run HEAD^ HEAD)\n\nis broken, too, since GIT_PERF_MAKE_OPTS wasn't set in the original make\ninvocation at all!\n\n> I do think it would be nice if environment variables took precedence\n> over the sourced GIT-BUILD-OPTIONS for \"./run\", but I suspect doing so\n> is a little tricky.\n\nYeah, I agree, and I think that would be tantamount to also fixing the\npre-4638e8806e behavior, which would be nice. I think a good middle\nground would be to continue to allow environment variables to override\noptions that are unset in GIT-BUILD-OPTIONS, which definitely is a\nregression in 4638e8806e.\n\n> > So I think a more robust fix might look like only filling out those\n> > lines in the GIT-BUILD-OPTIONS template when they are non-empty, similar\n> > to the pre-4638e8806e behavior. Something like:\n>\n> Yeah, that would fix the regression. But I kind of feel like your\n> initial command is already skirting the edges of what the original code\n> was meant to handle.\n\nHmm. I'm not sure I am following what you're saying here. How so?\n\nThanks,\nTaylor\n"},{"id":"513597","messageId":"20250305075710.GA1917508@coredump.intra.peff.net","threadId":"63026","inReplyTo":"Z8d5++1dNdo/32uz@nand.local","subject":"Re: GIT-BUILD-OPTIONS can override manual invocations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-05T07:57:10Z","receivedAt":"2025-03-05T07:57:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 04, 2025 at 05:08:59PM -0500, Taylor Blau wrote:\n\n> > Hmm. I am not sure what this is supposed to do, as I would not expect\n> > that \"make -C t/perf\" to build anything at all. It will use the working\n> > tree version built in the first step. So I'd expect your initial \"make\"\n> > to do all the work (and either fail or not depending on whether NO_EXPAT\n> > is set in your config.mak).\n> \n> Oops, I should have used the t/interop example instead of t/perf from\n> above. I agree that t/perf doesn't build anything. The buggy invocation\n> that Elijah and I noticed was:\n> \n>     make &&\n>     make -C t/interop GIT_INTEROP_MAKE_OPTS='...'\n\nAh, that makes much more sense.\n\nAnd while it's the same issue as my:\n\n> >   make &&\n> >   (cd t/perf && GIT_PERF_MAKE_OPTS=NO_EXPAT=1 ./run HEAD^ HEAD)\n\nI can see how it's even more confusing, since you are feeding it as a\nmake variable, and not through the environment (though of course it is\nthrough the environment that you'd eventually expect it to make it).\n\n> > I do think it would be nice if environment variables took precedence\n> > over the sourced GIT-BUILD-OPTIONS for \"./run\", but I suspect doing so\n> > is a little tricky.\n> \n> Yeah, I agree, and I think that would be tantamount to also fixing the\n> pre-4638e8806e behavior, which would be nice. I think a good middle\n> ground would be to continue to allow environment variables to override\n> options that are unset in GIT-BUILD-OPTIONS, which definitely is a\n> regression in 4638e8806e.\n\nIMHO the root of the problem is writing GIT_PERF_MAKE_OPTS and\nGIT_INTEROP_MAKE_OPTS into GIT-BUILD-OPTIONS at all. It is not used in\nthe actual build of Git run by the initial \"make\" invocation, and if it\nwere to change we would not actually need to rebuild anything. And yet:\n\n  make GIT_PERF_MAKE_OPTS=foo\n\nwill not fail (since we don't look at the nonsense value), and:\n\n  make GIT_PERF_MAKE_OPTS=bar\n\nwill rebuild at least scripts because it doesn't know that the changed\nvalue is uninteresting.\n\nIf we instead just read the value when running \"make -C t/interop\", we'd\nalways get the fresh value (whether from the command-line, the\nenvironment, or reading config.mak).\n\nBut there are two catches:\n\n  - right now if you do:\n\n      make GIT_INTEROP_MAKE_OPTS=whatever\n      make -C t/interop\n\n    it will use the value from the original \"make\". I think that is the\n    root of the confusion, but possibly people depend on that? I don't\n    know. I would (and in fact do) put it into my config.mak, where it\n    would be seen by both invocations.\n\n  - we'd want to see the values from both make and the shell, and their\n    syntaxes are slightly different. In particular, right now I'd do:\n\n      echo GIT_PERF_MAKE_OPTS=whatever >>config.mak\n      make  ;# this writes it into GIT-BUILD-OPTIONS\n      cd t/perf\n      ./run HEAD^ HEAD ;# this sources GIT-BUILD-OPTIONS\n\n    How does that work without GIT-BUILD-OPTIONS? I'd need ./run to\n    source the config.mak file, but it's in make format, not shell. So\n    I'd have to do something like this in the run script:\n\n      opts=$(make -epn | perl -lne 'print $1 if /^GIT_PERF_MAKE_OPTS = (.*)/')\n\n    (or there are variants that can be implemented with a special\n    target). It's an extra step, but maybe not too bad.\n\nSo I dunno. It may not be worth changing the status quo.\n\n> > > So I think a more robust fix might look like only filling out those\n> > > lines in the GIT-BUILD-OPTIONS template when they are non-empty, similar\n> > > to the pre-4638e8806e behavior. Something like:\n> >\n> > Yeah, that would fix the regression. But I kind of feel like your\n> > initial command is already skirting the edges of what the original code\n> > was meant to handle.\n> \n> Hmm. I'm not sure I am following what you're saying here. How so?\n\nI think you were lucky that giving a different GIT_PERF_MAKE_OPTS\nvariable to your second make invocation ever worked at all. It was not\nnot the intended mechanism (at least that is my understanding; like I\nsaid, most of this was not documented and the implications of various\napproaches were probably not through through all that much).\n\nSo yes, 4638e8806e is a regression for your particular invocation, but\ngiven the adjacent issues (if you had those variables set in your\nconfig.mak), is it worth trying to support your case? Or put another\nway, is there any reason you can't just do:\n\n  make GIT_INTEROP_MAKE_OPTS=whatever && make -C t/interop\n\n? If this change impacted a lot of users, I'd be more worried about\nretaining corner cases. But the perf and interop suites are developer\ntools, and I'd guess that only a tiny fraction of Git devs even use\nthem.\n\nNot that I am particularly opposed to your \"don't write empty values to\nthe GIT-BUILD-OPTIONS\" solution. My main complaint is just that it's new\nextra code, though if we can at least write it _once_ instead of having\nto remember to do it for each variable, that's not too bad.\n\n-Peff\n"}]}