diff mbox series

[v3,4/4] Makefile: simplify $(test_bindir_programs) rule by splitting it up

Message ID patch-v3-4.4-0ff09495476-20221031T222249Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series Makefile: untangle bin-wrappers/% rule complexity | expand

Commit Message

Ævar Arnfjörð Bjarmason Oct. 31, 2022, 10:28 p.m. UTC
When the @@PROG@@ substitution was added in [1] it was a simple matter
of doing a:

	's|@@PROG@@|$(@F)|'

Then when t/helpers were added in [2] followed by the ".exe" needing
to be appended in [3] this previously simple rule ended up as a dense
one-liner.

It has been pointed out that this is hard to read[4], but the problem
isn't the "design of the Makefile syntax". It's we now have to
after-the-fact determine if we were dealing with a bin-wrapper/ that
needed to have $(X) appended, a t/helper/, or a non-binary (currently
only git-cvsserver).

That would be a problem in any language. We're starting out with three
lists, and then end up having to heuristically determine given a
member of any of those lists which list that member came from. Let's
just stop doing that and keep track of what member belongs to which
list.

We can do this by splitting up the single "bin-wrappers/%" rule into a
rule for each of the three lists. With the
"cmd_munge_script_sed_shell_path_arg" define added in a preceding
commit this is easy, we just need to add a sister template to the
existing "cmd_munge_script" added in [5].

The "filter-out" added in [6] has now become unnecessary per the
above, it was an artifact of us losing track of what was in which of
our lists to begin with.

This change can be tested with e.g.:

	git checkout master &&
	make SHELL_PATH=/bin/bash X=.exe &&
	mv bin-wrappers bin-wrappers.master &&
	<apply this change> &&
	make SHELL_PATH=/bin/bash X=.exe &&
	diff -ru bin-wrappers{.master,}

Which will show an empty diff, i.e. we've correctly dealt with the
combination of $(SHELL_PATH), $(X) and these three variables here.

This also fixes an issue with the "bin-wrappers/" scripts have never
had properly declared dependency information, i.e. this has never
worked:

	make clean &&
	make bin-wrappers/git &&
	# the script is there, but no "./git" is built
	./bin-wrappers/git

There was no reason not to have that work, just as most things
generated by the Makefile have proper dependency information.

Before this commit doing this would have been painful, but now it's
easy to pass this as a parameter to our "bin_wrappers_template"

1. ea925196f1b (build dashless "bin-wrappers" directory similar to
   installed bindir, 2009-12-02)
2. e6e7530d10b (test helpers: move test-* to t/helper/ subdirectory,
   2016-04-13)
3. 3a94cb31d52 (bin-wrappers: append `.exe` to target paths if
   necessary, 2019-07-29)
4. https://lore.kernel.org/git/sso99so6-n28s-rq86-8q20-4456r3pn869r@tzk.qr/
5. 46bac904581 (Do not install shell libraries executable, 2010-01-31)
6. 7b5c93c6c68" (scalar: include in standard Git build & installation,
   2022-09-02)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 032654640c5..d7ab68e3db8 100644
--- a/Makefile
+++ b/Makefile
@@ -3067,16 +3067,42 @@  GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-BIN_WRAPPERS = $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+define cmd_munge_bin_wrappers_script
+sed \
+	-e $(call cmd_munge_script_sed_shell_path_arg) \
+	-e 's|@@BUILD_DIR@@|$(shell pwd)|' \
+	-e 's|@@PROG@@|$(2)$(1)$(3)|' \
+	<$< >$@ && \
+	chmod +x $@
+endef
 
-all:: $(BIN_WRAPPERS)
+define bin_wrappers_template
+
+### bin_wrappers_template; Parameters:
+## E.g. "BINDIR_PROGRAMS_NEED_X": Variable reference
+# 1='$(1)'
+## E.g. "$(@F)": Passed as $$(1)) to "cmd_munge_bin_wrappers_script"
+# 2='$(2)'
+## E.g. "" or "t/helper": Directory prefix for the wrapped binary
+# 3='$(3)'
+## E.g. "" or "$$X": If $$X: wrapped binary needs X=.exe (for Windows)
+# 4='$(4)'
+BW_$(1) = $$($(1):%=bin-wrappers/%)
+BIN_WRAPPERS += $$(BW_$(1))
+$$(BW_$(1)): bin-wrappers/% : $(3)%$(4)
+$$(BW_$(1)): wrap-for-bin.sh
+	$$(call mkdir_p_parent_template)
+	$$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4))
+endef
 
-bin-wrappers/%: wrap-for-bin.sh
-	$(call mkdir_p_parent_template)
-	$(QUIET_GEN)sed -e $(call cmd_munge_script_sed_shell_path_arg) \
-	     -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
-	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
-	chmod +x $@
+define bin_wrappers_templates
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X)
+$(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)')
+$(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
+endef
+$(eval $(call bin_wrappers_templates))
+
+all:: $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems