GIT-BUILD-OPTIONS can override manual invocations

Commit Message

Taylor Blau Feb. 28, 2025, 8:08 p.m. UTC

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

Patch

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