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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 26, 2022, 19:14 UTC
Message-ID: <221026.86tu3qbd03.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqr0yuh144.fsf@gitster.g>

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

Show 11 quoted lines

> Æ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...]

Show 5 quoted lines

> 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).

Show 11 quoted lines

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