git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Victoria Dye" <vdye@github.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH 3/5] Makefile: simplify $(test_bindir_programs) rule by splitting it up
Date: Thu,  1 Sep 2022 15:17:26 +0200	[thread overview]
Message-ID: <patch-3.5-0f66992d5f7-20220901T130817Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.5-00000000000-20220901T130817Z-avarab@gmail.com>

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

There is then an an in-flight change[6] that generalizes this to
support "scalar". That change needed to further special-case the
pre-image here, as we couldn't rely on the "git%" heuristic to mean
"not from the $(TEST_PROGRAMS_NEED_X) variable".

With this conflicting change that special-casing becomes
unnecessary. The conflict between the two can be resolved entirely in
favor of this change, as the other change adds "scalar" to
"$(BINDIR_PROGRAMS_NEED_X)", and we are no longer losing track of
which members belong to that list.

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.

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. https://lore.kernel.org/git/4d69e5eaccb8873eece666a3d2bb2b22abdde7ea.1661961746.git.gitgitgadget@gmail.com/

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

diff --git a/Makefile b/Makefile
index 0d6b2caa7d9..96de9e55864 100644
--- a/Makefile
+++ b/Makefile
@@ -597,6 +597,7 @@ export TCL_PATH TCLTK_PATH
 PTHREAD_LIBS = -lpthread
 
 # Guard against environment variables
+BIN_WRAPPERS =
 BUILTIN_OBJS =
 BUILT_INS =
 COMPAT_CFLAGS =
@@ -3061,16 +3062,34 @@ GIT-PYTHON-VARS: FORCE
             fi
 endif
 
-test_bindir_programs := $(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:: $(test_bindir_programs)
+BW_BINDIR_PROGRAMS_NEED_X = $(BINDIR_PROGRAMS_NEED_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NEED_X)
+$(BW_BINDIR_PROGRAMS_NEED_X): wrap-for-bin.sh
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),,$X)
 
-bin-wrappers/%: wrap-for-bin.sh
+BW_BINDIR_PROGRAMS_NO_X = $(BINDIR_PROGRAMS_NO_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_BINDIR_PROGRAMS_NO_X)
+$(BW_BINDIR_PROGRAMS_NO_X): 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-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
-	chmod +x $@
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F))
+
+BW_TEST_PROGRAMS_NEED_X = $(TEST_PROGRAMS_NEED_X:%=bin-wrappers/%)
+BIN_WRAPPERS += $(BW_TEST_PROGRAMS_NEED_X)
+$(BW_TEST_PROGRAMS_NEED_X): wrap-for-bin.sh
+	$(call mkdir_p_parent_template)
+	$(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,$(@F),t/helper/,$X)
+
+all:: $(BIN_WRAPPERS)
 
 # GNU make supports exporting all variables by "export" without parameters.
 # However, the environment gets quite big, and some programs have problems
@@ -3403,7 +3422,7 @@ OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
 artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
-		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
+		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(BIN_WRAPPERS) \
 		$(MOFILES)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
-- 
2.37.3.1426.g360dd7cf8ca


  parent reply	other threads:[~2022-09-01 13:20 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-31 16:02 [PATCH 0/8] scalar: integrate into core Git Victoria Dye via GitGitGadget
2022-08-31 16:02 ` [PATCH 1/8] scalar: fix command documentation section header Victoria Dye via GitGitGadget
2022-08-31 16:02 ` [PATCH 2/8] scalar: include in standard Git build & installation Victoria Dye via GitGitGadget
2022-09-01  9:11   ` Johannes Schindelin
2022-09-01 13:17     ` [PATCH 0/5] Makefile: split up $(test_bindir_programs) Ævar Arnfjörð Bjarmason
2022-09-01 13:17       ` [PATCH 1/5] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-09-01 13:17       ` [PATCH 2/5] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-09-01 13:17       ` Ævar Arnfjörð Bjarmason [this message]
2022-09-01 13:17       ` [PATCH 4/5] Makefile: define bin-wrappers/% rules with a template Ævar Arnfjörð Bjarmason
2022-09-01 13:17       ` [PATCH 5/5] Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies Ævar Arnfjörð Bjarmason
2022-09-01 15:02       ` [PATCH 0/5] Makefile: split up $(test_bindir_programs) Derrick Stolee
2022-09-02 12:38       ` Johannes Schindelin
2022-10-26 14:42       ` [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule Ævar Arnfjörð Bjarmason
2022-10-26 14:42         ` [PATCH v2 1/3] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-10-26 17:51           ` Junio C Hamano
2022-10-26 14:42         ` [PATCH v2 2/3] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-10-26 16:47           ` Junio C Hamano
2022-10-26 18:47             ` Ævar Arnfjörð Bjarmason
2022-10-26 19:13               ` Junio C Hamano
2022-10-26 14:42         ` [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up Ævar Arnfjörð Bjarmason
2022-10-26 18:48           ` Junio C Hamano
2022-10-26 19:14             ` Ævar Arnfjörð Bjarmason
2022-10-26 20:28               ` Junio C Hamano
2022-10-26 20:43                 ` Ævar Arnfjörð Bjarmason
2022-10-28  0:57         ` [PATCH v2 0/3] Makefile: fix issues with bin-wrappers/% rule Jeff King
2022-10-28  3:03           ` Ævar Arnfjörð Bjarmason
2022-10-31 22:28         ` [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity Ævar Arnfjörð Bjarmason
2022-10-31 22:28           ` [PATCH v3 1/4] Makefile: factor sed-powered '#!/bin/sh' munging into a variable Ævar Arnfjörð Bjarmason
2022-10-31 22:28           ` [PATCH v3 2/4] Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Ævar Arnfjörð Bjarmason
2022-10-31 22:28           ` [PATCH v3 3/4] Makefile: rename "test_bindir_programs" variable, pre-declare Ævar Arnfjörð Bjarmason
2022-10-31 22:28           ` [PATCH v3 4/4] Makefile: simplify $(test_bindir_programs) rule by splitting it up Ævar Arnfjörð Bjarmason
2022-10-31 23:54           ` [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity Taylor Blau
2022-11-01  1:29             ` Ævar Arnfjörð Bjarmason
2022-08-31 16:02 ` [PATCH 3/8] git help: special-case `scalar` Johannes Schindelin via GitGitGadget
2022-08-31 16:02 ` [PATCH 4/8] scalar: implement the `help` subcommand Johannes Schindelin via GitGitGadget
2022-08-31 16:48   ` Ævar Arnfjörð Bjarmason
2022-09-01 16:08     ` Victoria Dye
2022-09-01  8:51   ` Johannes Schindelin
2022-09-01  9:17   ` Johannes Schindelin
2022-08-31 16:02 ` [PATCH 5/8] scalar-clone: add test coverage Victoria Dye via GitGitGadget
2022-09-01  9:32   ` Johannes Schindelin
2022-09-01 23:49     ` Victoria Dye
2022-09-02  9:07       ` Johannes Schindelin
2022-09-02 16:52         ` Junio C Hamano
2022-08-31 16:02 ` [PATCH 6/8] t/perf: add Scalar performance tests Victoria Dye via GitGitGadget
2022-09-01  9:39   ` Johannes Schindelin
2022-09-01 16:15     ` Victoria Dye
2022-09-01 16:21       ` Victoria Dye
2022-09-02  9:16         ` Johannes Schindelin
2022-09-01 16:43   ` Junio C Hamano
2022-08-31 16:02 ` [PATCH 7/8] t/perf: add 'GIT_PERF_USE_SCALAR' run option Victoria Dye via GitGitGadget
2022-09-01  9:43   ` Johannes Schindelin
2022-09-02  4:00     ` Victoria Dye
2022-09-02  9:17       ` Johannes Schindelin
2022-08-31 16:02 ` [PATCH 8/8] Documentation/technical: include Scalar technical doc Victoria Dye via GitGitGadget
2022-08-31 17:03 ` [PATCH 0/8] scalar: integrate into core Git Ævar Arnfjörð Bjarmason
2022-08-31 18:42   ` Victoria Dye
2022-09-01  9:56 ` Johannes Schindelin
2022-09-02 15:56 ` [PATCH v2 0/9] " Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 1/9] scalar: fix command documentation section header Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 2/9] scalar: include in standard Git build & installation Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 3/9] git help: special-case `scalar` Johannes Schindelin via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 4/9] scalar: implement the `help` subcommand Johannes Schindelin via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 5/9] scalar: add to 'git help -a' command list Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 6/9] scalar-clone: add test coverage Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 7/9] t/perf: add Scalar performance tests Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 8/9] t/perf: add 'GIT_PERF_USE_SCALAR' run option Victoria Dye via GitGitGadget
2022-09-02 15:56   ` [PATCH v2 9/9] Documentation/technical: include Scalar technical doc Victoria Dye via GitGitGadget
2022-09-05 10:36   ` [PATCH v2 0/9] scalar: integrate into core Git Johannes Schindelin
2022-09-08 20:54   ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-3.5-0f66992d5f7-20220901T130817Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).