Re: [PATCH v3 0/4] Makefile: untangle bin-wrappers/% rule complexity

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 1, 2022, 01:29 UTC
Message-ID: <221101.86edun5tgn.gmgdl@evledraar.gmail.com>
In-Reply-To: <Y2BgRU+HnJjWPzkn@nand.local>

On Mon, Oct 31 2022, Taylor Blau wrote:

Show 21 quoted lines

> 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/