| Message ID | patch-v2-2.3-6dcb49f25c4-20221026T143534Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Makefile: fix issues with bin-wrappers/% rule | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Define the variables that make up TEST_OBJS earlier, and don't go back > & forth in their definition. Before we'd first append $X to > $(TEST_PROGRAMS), and then substitute $X back out of it to define > $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, > which avoids this needless back & forth substitution. Makes sense, I guess. So TEST_OBJS is no longer used? > TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) > +all:: $(TEST_PROGRAMS) This change is not necessary to achieve the stated goal of this step, though. It is one of those "while at it" distraction that consumes our already constrained reviewer bandwidth, no? Having said that, "all::" being able to be built up with independent pieces shine here in this split from the original. It probably is easier to reason about while seeing this isolated area of Makefile what is being done to TEST_PROGRAMS. The rest of the patch is quite straight-forward renaming of TEST_OBJS to TEST_PROGRAM_OBJS and an improvement of how the elements on the list are computed from the source-of-truth list that is TEST_PROGRAMS_NEED_X that looks correct. > +TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X)) > +.PRECIOUS: $(TEST_PROGRAM_OBJS) > # List built-in command $C whose implementation cmd_$C() is not in > # builtin/$C.o but is linked in as part of some other command. > @@ -2543,10 +2548,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o > REFTABLE_TEST_OBJS += reftable/test_framework.o > REFTABLE_TEST_OBJS += reftable/tree_test.o > > -TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) > - > .PHONY: test-objs > -test-objs: $(TEST_OBJS) > +test-objs: $(TEST_PROGRAM_OBJS) > > GIT_OBJS += $(LIB_OBJS) > GIT_OBJS += $(BUILTIN_OBJS) > @@ -2562,7 +2565,7 @@ scalar-objs: $(SCALAR_OBJS) > OBJECTS += $(GIT_OBJS) > OBJECTS += $(SCALAR_OBJS) > OBJECTS += $(PROGRAM_OBJS) > -OBJECTS += $(TEST_OBJS) > +OBJECTS += $(TEST_PROGRAM_OBJS) > OBJECTS += $(XDIFF_OBJS) > OBJECTS += $(FUZZ_OBJS) > OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) > @@ -3061,7 +3064,7 @@ endif > > test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X)) > > -all:: $(TEST_PROGRAMS) $(test_bindir_programs) > +all:: $(test_bindir_programs) > > bin-wrappers/%: wrap-for-bin.sh > $(call mkdir_p_parent_template) > @@ -3087,8 +3090,6 @@ perf: all > > .PHONY: test perf > > -.PRECIOUS: $(TEST_OBJS) > - > t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) > > t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
On Wed, Oct 26 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Define the variables that make up TEST_OBJS earlier, and don't go back >> & forth in their definition. Before we'd first append $X to >> $(TEST_PROGRAMS), and then substitute $X back out of it to define >> $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, >> which avoids this needless back & forth substitution. > > Makes sense, I guess. So TEST_OBJS is no longer used? Yes, sorry I'll clarify that in a re-roll. >> TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) >> +all:: $(TEST_PROGRAMS) > > This change is not necessary to achieve the stated goal of this > step, though. It is one of those "while at it" distraction that > consumes our already constrained reviewer bandwidth, no? I figured this would be better use of that bandwith, since the reviewer doesn't need to wonder why these are still spread befo/after the main body of the change. Not everyone is keenly aware of the at first odd way a Makefile is read (per "3.7 How 'make' Reads a Makefile" in the GNU make manual). But I'm happy to eject this part if that helps, but... > Having said that, "all::" being able to be built up with independent > pieces shine here in this split from the original. It probably is > easier to reason about while seeing this isolated area of Makefile > what is being done to TEST_PROGRAMS. ...here I'm not quite sure if you want to keep it after all or nat... > The rest of the patch is quite straight-forward renaming of > TEST_OBJS to TEST_PROGRAM_OBJS and an improvement of how the > elements on the list are computed from the source-of-truth list that > is TEST_PROGRAMS_NEED_X that looks correct. Thanks for the quick review!
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Oct 26 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> Define the variables that make up TEST_OBJS earlier, and don't go back >>> & forth in their definition. Before we'd first append $X to >>> $(TEST_PROGRAMS), and then substitute $X back out of it to define >>> $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, >>> which avoids this needless back & forth substitution. >> >> Makes sense, I guess. So TEST_OBJS is no longer used? > > Yes, sorry I'll clarify that in a re-roll. It invites the question why it needed to be renamed in the first place, though. If we needed to name something that is different from the original TEST_OBJS, it makes perfect sense to add a variable with a new name, but if it is not the case here, then...? >> Having said that, "all::" being able to be built up with independent >> pieces shine here in this split from the original. It probably is >> easier to reason about while seeing this isolated area of Makefile >> what is being done to TEST_PROGRAMS. > > ...here I'm not quite sure if you want to keep it after all or nat... I'll let it pass this time ;-) but my patience is not infinite.
diff --git a/Makefile b/Makefile index 45b22d33513..a8cfa096dc1 100644 --- a/Makefile +++ b/Makefile @@ -617,7 +617,8 @@ SCRIPT_PYTHON = SCRIPT_SH = SCRIPT_LIB = TEST_BUILTINS_OBJS = -TEST_OBJS = +TEST_PROGRAMS = +TEST_PROGRAM_OBJS = TEST_PROGRAMS_NEED_X = THIRD_PARTY_SOURCES = @@ -795,6 +796,7 @@ TEST_BUILTINS_OBJS += test-wildmatch.o TEST_BUILTINS_OBJS += test-windows-named-pipe.o TEST_BUILTINS_OBJS += test-write-cache.o TEST_BUILTINS_OBJS += test-xml-encode.o +TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) # Do not add more tests here unless they have extra dependencies. Add # them in TEST_BUILTINS_OBJS above. @@ -802,6 +804,9 @@ TEST_PROGRAMS_NEED_X += test-fake-ssh TEST_PROGRAMS_NEED_X += test-tool TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X)) +all:: $(TEST_PROGRAMS) +TEST_PROGRAM_OBJS += $(patsubst %,t/helper/%.o,$(TEST_PROGRAMS_NEED_X)) +.PRECIOUS: $(TEST_PROGRAM_OBJS) # List built-in command $C whose implementation cmd_$C() is not in # builtin/$C.o but is linked in as part of some other command. @@ -2543,10 +2548,8 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o REFTABLE_TEST_OBJS += reftable/test_framework.o REFTABLE_TEST_OBJS += reftable/tree_test.o -TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) - .PHONY: test-objs -test-objs: $(TEST_OBJS) +test-objs: $(TEST_PROGRAM_OBJS) GIT_OBJS += $(LIB_OBJS) GIT_OBJS += $(BUILTIN_OBJS) @@ -2562,7 +2565,7 @@ scalar-objs: $(SCALAR_OBJS) OBJECTS += $(GIT_OBJS) OBJECTS += $(SCALAR_OBJS) OBJECTS += $(PROGRAM_OBJS) -OBJECTS += $(TEST_OBJS) +OBJECTS += $(TEST_PROGRAM_OBJS) OBJECTS += $(XDIFF_OBJS) OBJECTS += $(FUZZ_OBJS) OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS) @@ -3061,7 +3064,7 @@ endif test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X)) -all:: $(TEST_PROGRAMS) $(test_bindir_programs) +all:: $(test_bindir_programs) bin-wrappers/%: wrap-for-bin.sh $(call mkdir_p_parent_template) @@ -3087,8 +3090,6 @@ perf: all .PHONY: test perf -.PRECIOUS: $(TEST_OBJS) - t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB)
Define the variables that make up TEST_OBJS earlier, and don't go back & forth in their definition. Before we'd first append $X to $(TEST_PROGRAMS), and then substitute $X back out of it to define $(TEST_OBJS). Let's instead add a new $(TEST_PROGRAM_OBJS) variable, which avoids this needless back & forth substitution. See daa99a91729 (Makefile: make sure test helpers are rebuilt when headers change, 2010-01-26) for how we ended up with the original $(TEST_OBJS). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)