| Message ID | cover-v2-0.3-00000000000-20221026T143533Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Makefile: fix issues with bin-wrappers/% rule | expand |
On Wed, Oct 26, 2022 at 04:42:34PM +0200, Ævar Arnfjörð Bjarmason wrote: > This simple topic fixes issues with the bin-wrappers/% rules, as seen > in the range-diff below this never worked properly: > > make bin-wrappers/git > > I.e. we'd make the scirpt, but not "git". Maybe I'm missing something, but the current behavior is what I'd expect to happen. Sure, bin-wrappers/git depends on "git" at run-time if you want it to do something useful. But AFAIK it doesn't at build-time. Why do we want a build-time dependency? Are we expecting rules to depend on bin-wrappers/foo but not also on foo? Or people to ask manually for those targets? This kind of feels like making git-repack depend on git-pack-objects. One will call the other at run-time, but there's no reason the builds have to be together. And if you try to build and run one without building the other, well...if it hurts, don't do it. -Peff
On Thu, Oct 27 2022, Jeff King wrote: > On Wed, Oct 26, 2022 at 04:42:34PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> This simple topic fixes issues with the bin-wrappers/% rules, as seen >> in the range-diff below this never worked properly: >> >> make bin-wrappers/git >> >> I.e. we'd make the scirpt, but not "git". > > Maybe I'm missing something, but the current behavior is what I'd expect > to happen. Sure, bin-wrappers/git depends on "git" at run-time if you > want it to do something useful. But AFAIK it doesn't at build-time. Why > do we want a build-time dependency? Are we expecting rules to depend on > bin-wrappers/foo but not also on foo? Or people to ask manually for > those targets? I didn't have an immediate practical reason for it beyond just not leaving holes in the dependency DAG. But yes, nothing is only building bin-wrappers/% now, so nothing needs this. It just helps to not have to worry about Makefile targets not needing other targets to work. > This kind of feels like making git-repack depend on git-pack-objects. > One will call the other at run-time, but there's no reason the builds > have to be together. And if you try to build and run one without > building the other, well...if it hurts, don't do it. Or wget depending on your router being plugged in? Beacuse that's what you use it for?:) I get the general point, but I don't think these are comparable to those sorts of loose or semi-loose dependencies. For repack it'll service up e.g. "-h" without pack-objects, at least that's something... Whereas these scripts all end with invoking the binary corresponding to their name, so they really are useless without that target. So I think it makes sense to make it a dependency of the target in the Makefile.
This simple topic fixes issues with the bin-wrappers/% rules, as seen in the range-diff below this never worked properly: make bin-wrappers/git I.e. we'd make the scirpt, but not "git". I originally sent this as [1] in response to a topic that's since landed, i.e. that topic needed to add more conditions to the "match this, but not that" part of the current rule. As 3/3 notes that's because we were previously squashing 3 Makefile variables into one, and then having to heurisitcally match their contents to figure out which item originally came from which variable. The 3/3 here simply avoids squashing all that data together, so we don't need to guess after the fact. For the v2 I squashed the previous 3 commits together, they were incrementally improving the rule, now we do it all at once. 1. https://lore.kernel.org/git/cover-0.5-00000000000-20220901T130817Z-avarab@gmail.com/ Ævar Arnfjörð Bjarmason (3): Makefile: factor sed-powered '#!/bin/sh' munging into a variable Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier Makefile: simplify $(test_bindir_programs) rule by splitting it up Makefile | 67 ++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 19 deletions(-) Range-diff against v1: 1: 40033143cdd = 1: fc6c5a6a8df Makefile: factor sed-powered '#!/bin/sh' munging into a variable 2: fe54dacaad2 = 2: 6dcb49f25c4 Makefile: define "TEST_{PROGRAM,OBJS}" variables earlier 3: 9d4ac628f0c ! 3: 400f487e30d 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: + + 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, @@ Makefile: GIT-PYTHON-VARS: FORCE +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) ++define bin_wrappers_template ++ ++## bin_wrappers_template ++# 1 = $(1) ++# 2 = $(2) ++# 3 = $(3) ++# 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) ++ $$(QUIET_GEN)$$(call cmd_munge_bin_wrappers_script,$(2),$(3),$(4)) ++endef -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) +- $(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 $@ -+ $(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) ++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) 4: cbbf458433f < -: ----------- Makefile: define bin-wrappers/% rules with a template 5: 560dee80b4a < -: ----------- Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies