Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Victoria Dye <vdye@github.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 3/3] Makefile: simplify $(test_bindir_programs) rule by splitting it up
Date: Wed, 26 Oct 2022 21:14:29 +0200	[thread overview]
Message-ID: <221026.86tu3qbd03.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <xmqqr0yuh144.fsf@gitster.g>

On Wed, Oct 26 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> -test_bindir_programs := $(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) \
>> +	-e 's|@@BUILD_DIR@@|$(shell pwd)|' \
>> +	-e 's|@@PROG@@|$(2)$(1)$(3)|' \
>> +	<$< >$@ && \
>> +	chmod +x $@
>> +endef

[Re-arranged your question a bit for the reply...]

> We've talked about relying on another feature of GNU make (which we
> already depend on) to automatically remove the target build artifact
> when a rule to build it fails, [...]  What happens when a rule that
> uses the above fails in the middle?  Do we correctly remove the
> half-built target?

Are you asking if "define"'s execute in some context outside the purview
of rules, so that if you $(call) one from within a rule and it errors,
that we won't clean up the file?

No, that won't happen, they'll be cleaned up properly. First, these are
basically macros, but secondly it wouldn't matter if they
weren't. E.g. you can execute an arbitrary program that happens to write
to a file that matches your $@.

The only thing that matters is that we carry forward the exit code,
which we do here. I.e. the "&&" after the $(call).

(Even then we'd still error properly, unless you ran "make -i" to ignore
errors in the rule, the && is more in service of the $(QUIET_*) in our
Makefile).

> but I see quite a many old world best practice pattern "generate >$@+
> && chmod +x $@+ && mv $@+ $@" still in today's Makefile.

Those are mostly there because nobody's cleaned them up, we don't need
them anymore.

"Mostly" because one thing they still do is not replace the file until
it's ready, we had a previous discussion on a related topic in reply to
https://lore.kernel.org/git/patch-1.6-3330cdbccc0-20210329T161723Z-avarab@gmail.com/
(it didn't go in).

>> -all:: $(test_bindir_programs)
>> +define bin_wrappers_template
>> +
>> +## bin_wrappers_template
>> +# 1 = $(1)
>> +# 2 = $(2)
>> +# 3 = $(3)
>> +# 4 = $(4)
>
> Whatever the above comment wants to convey to readers, it seems to
> fail to do so at least to me, while ...

I can drop them, FWIW I've found it quite handy to add these to ad-hoc
debug templates. E.g. you can run:

	$ make -f /dev/null -E '$(eval $(file <Makefile))' -E '$(error $(call bin_wrappers_template,a,b,c,d))'
	make: *** 
	## bin_wrappers_template
	# 1 = a
	# 2 = b
	# 3 = c
	# 4 = d
	BW_a = $(a:%=bin-wrappers/%)
	BIN_WRAPPERS += $(BW_a)
	all:: $(BW_a)
	$(BW_a): bin-wrappers/% : c%d
	$(BW_a): wrap-for-bin.sh
	        $(call mkdir_p_parent_template)
	        $(QUIET_GEN)$(call cmd_munge_bin_wrappers_script,b,c,d).  Stop.

So you see what the parameters expand to. Maybe just changing the
heading to:

	## bin_wrappers_template: $(1..N) below for manual "$(error $(call ..." deubgging

?