[0/5] Makefile: split up $(test_bindir_programs)

Message

Ævar Arnfjörð Bjarmason Sept. 1, 2022, 1:17 p.m. UTC

On Thu, Sep 01 2022, Johannes Schindelin wrote:

> [...]
> On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> [...]
>> @@ -3062,7 +3067,7 @@ bin-wrappers/%: wrap-for-bin.sh
>>  	$(call mkdir_p_parent_template)
>>  	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>>  	     -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)))|' < $< > $@ && \
>> +	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
>
> It took me a good while to wrap my head around this (and let me be clear:
> I consider none of this your fault, it's the fault of the design of the
> Makefile syntax).
>
> Let me untangle this, for posterity's benefit. We substitute the
> placeholder `@@PROG@@` with a concatenation of two strings that are both
> derived from `@F`, i.e. the basename of the to-be-wrapped command.

We could do this later, but the 3/5 here is my reply to the "fault of
the design of the Makfile syntax".

I really don't think that's the case, the problem here is something
you'd get any any language.

We have three lists which we'd like to treat differently, but for no
particularly good reason other than incrementally building on past
changes to end up with this we end up having to on-the-fly guess which
list a given item came from.

With this series the end result is instead to do:
	
	define bin_wrappers_template
	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
	
	$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X))
	$(eval $(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)'))
	$(eval $(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X))
	
	all:: $(BIN_WRAPPERS)

This obviously conflicts with Victoria's changes here, but if picked
up the conflict can be entirely solved in favor of this series, and
this "scalar" series will benefit.

I.e. the only reason this series needs to patch this one liner is
because the Makefile is losing track of where the item(s) came from.

Once we're not doing that we're perfectly capable of creating a
bin-wrappers/scalar, because we're no longer running into the logic
that uses git% as a heuristic to determine whether something is "not
from the $(TEST_PROGRAMS_NEED_X) variable".

A CI run, showing that this also works on Windows etc.:
https://github.com/avar/git/runs/8135048620

Ævar Arnfjörð Bjarmason (5):
  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: define bin-wrappers/% rules with a template
  Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies

 Makefile | 58 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 19 deletions(-)

Comments

Derrick Stolee Sept. 1, 2022, 3:02 p.m. UTC | #1

On 9/1/2022 9:17 AM, Ævar Arnfjörð Bjarmason wrote:

> Ævar Arnfjörð Bjarmason (5):
>   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: define bin-wrappers/% rules with a template
>   Makefile: fix "make clean && make bin-wrappers/$NAME" dependencies
> 
>  Makefile | 58 +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 19 deletions(-)
 
I find it very distracting when you send a full patch series in the
middle of someone else's patch series. Viewing the thread now involves
two distinct conversations, assuming that your series gets review and
requires multiple versions.

I would rather that you started a new thread with your proposed series
and then sent a reply in this thread pointing to that one.

Thanks,
-Stolee

Johannes Schindelin Sept. 2, 2022, 12:38 p.m. UTC | #2

Hi Ævar,

On Thu, 1 Sep 2022, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Sep 01 2022, Johannes Schindelin wrote:
>
> > [...]
> > On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> > [...]
> >> @@ -3062,7 +3067,7 @@ bin-wrappers/%: wrap-for-bin.sh
> >>  	$(call mkdir_p_parent_template)
> >>  	$(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> >>  	     -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)))|' < $< > $@ && \
> >> +	     -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
> >
> > It took me a good while to wrap my head around this (and let me be clear:
> > I consider none of this your fault, it's the fault of the design of the
> > Makefile syntax).
> >
> > Let me untangle this, for posterity's benefit. We substitute the
> > placeholder `@@PROG@@` with a concatenation of two strings that are both
> > derived from `@F`, i.e. the basename of the to-be-wrapped command.
>
> We could do this later, [... something about touching the same code ...]

Yes, and we should.

It would be better if we kept the focus on Scalar in this mail thread, to
get it done first.

It is the stated preference in this project anyway to avoid working on the
code that is already being worked on in other in-flight patch series, to
reduce unnecessary friction.

Ciao,
Johannes