| Message ID | cover-v3-0.4-00000000000-20221031T222249Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Makefile: untangle bin-wrappers/% rule complexity | expand |
On Mon, Oct 31, 2022 at 11:28:05PM +0100, Ævar Arnfjörð Bjarmason wrote: > Ævar Arnfjörð Bjarmason (4): > Makefile: factor sed-powered '#!/bin/sh' munging into a variable > Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier > Makefile: rename "test_bindir_programs" variable, pre-declare > Makefile: simplify $(test_bindir_programs) rule by splitting it up > > Makefile | 70 +++++++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 51 insertions(+), 19 deletions(-) Thanks. I replaced the earlier round with this one and pushed the result out to ttaylorr/git. But having read the topic over again, I have to say that I also find the pre-existing behavior to be as expected. "make bin-wrappers/git" produces its target as expected, but the target is useless because the relationship it has with "git" is a runtime dependency, not a Make-time dependency. So I'm not inclined to pick this one up, honestly. Perhaps other reviewers feel differently. Thanks, Taylor
On Mon, Oct 31 2022, Taylor Blau wrote: > On Mon, Oct 31, 2022 at 11:28:05PM +0100, Ævar Arnfjörð Bjarmason wrote: >> Ævar Arnfjörð Bjarmason (4): >> Makefile: factor sed-powered '#!/bin/sh' munging into a variable >> Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier >> Makefile: rename "test_bindir_programs" variable, pre-declare >> Makefile: simplify $(test_bindir_programs) rule by splitting it up >> >> Makefile | 70 +++++++++++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 51 insertions(+), 19 deletions(-) > > Thanks. I replaced the earlier round with this one and pushed the result > out to ttaylorr/git. > > But having read the topic over again, I have to say that I also find the > pre-existing behavior to be as expected. "make bin-wrappers/git" > produces its target as expected, but the target is useless because the > relationship it has with "git" is a runtime dependency, not a Make-time > dependency. > > So I'm not inclined to pick this one up, honestly. Perhaps other > reviewers feel differently. I'm fine with changing that. The part where it makes: make clean && make bin-wrappers/git && ./git ... work is a 1-line change to remove, i.e.: diff --git a/Makefile b/Makefile index d7ab68e3db8..d3c0a66b4b4 100644 --- a/Makefile +++ b/Makefile @@ -3089,7 +3089,6 @@ define bin_wrappers_template # 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)) This topic is mainly about untangling the dense $(filter) mess in the existing rule, which last came up as we need do to special-case "scalar", and we'd need to special-case any future binaries. But I see it's my own fault for making that clear, the "as seen in the range-diff below" in v2's CL [1] was meant to refer to "here's what's new in v2", not the topic's reason for existing. So, while I'm happy to re-roll it and remove that one line (and the associated part of the commit message) I'm surprised that there's such a hang-up about this aspect of it, to the point where I think I'm missing something. Nothing depends on "bin-wrappers/%" now, so having this harms nothing, but just seems to me to nicely tie off a loose end. I'm aware that e.g. git-submodule which depends on git-submodule.sh doesn't depend on e.g. "submodule--helper" being built, so in that sense this is inconsistent with the rest of our shellscripts. But in the case of wrap-for-bin.sh its generation is driven by the Makefile, and it's *only* there to to exec() the target binary (at least some of those other *.sh's can serve up -h on their own). And if we don't include that line, then (like on "master"): $ make clean >/dev/null; make bin-wrappers/git; ./git; echo $? GIT_VERSION = 2.38.GIT-dev MKDIR -p bin-wrappers GEN bin-wrappers/git bash: ./git: No such file or directory 127 But *shrug*, I just don't see how it's useful, when it's so easy to make it do something useful. 1. https://lore.kernel.org/git/cover-v2-0.3-00000000000-20221026T143533Z-avarab@gmail.com/
This series untangles the gnarly rule we use to generate bin-wrappers/%. It's now complex because we generate it from 3x separate variables, and then end up having to do inline pattern matching to decide which value comes from where. Instead, we can avoid squashing those values together, so we don't have to guess. See[1] for the v2. This hopefully addresses all outstanding issues/points that were raised. An added 3/4 here makes the eventual 4/4 smaler. 1. https://lore.kernel.org/git/cover-v2-0.3-00000000000-20221026T143533Z-avarab@gmail.com/ Ævar Arnfjörð Bjarmason (4): Makefile: factor sed-powered '#!/bin/sh' munging into a variable Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Makefile: rename "test_bindir_programs" variable, pre-declare Makefile: simplify $(test_bindir_programs) rule by splitting it up Makefile | 70 +++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 19 deletions(-) Range-diff against v2: 1: fc6c5a6a8df = 1: c9ce5b78a3a Makefile: factor sed-powered '#!/bin/sh' munging into a variable 2: 6dcb49f25c4 = 2: b95c296b6de Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier -: ----------- > 3: fea93c45898 Makefile: rename "test_bindir_programs" variable, pre-declare 3: 400f487e30d ! 4: 0ff09495476 Makefile: simplify $(test_bindir_programs) rule by splitting it up @@ Commit message 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: + 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 && @@ Commit message Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Makefile ## -@@ Makefile: export TCL_PATH TCLTK_PATH - PTHREAD_LIBS = -lpthread - - # Guard against environment variables -+BIN_WRAPPERS = - BUILTIN_OBJS = - BUILT_INS = - COMPAT_CFLAGS = @@ Makefile: GIT-PYTHON-VARS: FORCE fi endif --test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X)) +-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) \ @@ Makefile: GIT-PYTHON-VARS: FORCE + chmod +x $@ +endef --all:: $(test_bindir_programs) +-all:: $(BIN_WRAPPERS) +define bin_wrappers_template + -+## bin_wrappers_template -+# 1 = $(1) -+# 2 = $(2) -+# 3 = $(3) -+# 4 = $(4) ++### 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)) -+all:: $$(BW_$(1)) +$$(BW_$(1)): bin-wrappers/% : $(3)%$(4) +$$(BW_$(1)): wrap-for-bin.sh + $$(call mkdir_p_parent_template) @@ Makefile: GIT-PYTHON-VARS: FORCE # GNU make supports exporting all variables by "export" without parameters. # However, the environment gets quite big, and some programs have problems -@@ Makefile: 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)'