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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 26, 2022, 18:48 UTC
Message-ID: <xmqqr0yuh144.fsf@gitster.g>
In-Reply-To: <patch-v2-3.3-400f487e30d-20221026T143534Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 9 quoted lines

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

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, but I see quite a many old world best practice pattern "generate >$@+ && chmod +x $@+ && mv $@+ $@" still in today's Makefile. What happens when a rule that uses the above fails in the middle? Do we correctly remove the half-built target?

Show 8 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 ...

Show 17 quoted lines

> +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
>  
> +define bin_wrappers_templates
> +$(call bin_wrappers_template,BINDIR_PROGRAMS_NEED_X,'$$(@F)',,$$X)
> +$(call bin_wrappers_template,BINDIR_PROGRAMS_NO_X,'$$(@F)')
> +$(call bin_wrappers_template,TEST_PROGRAMS_NEED_X,'$$(@F)',t/helper/,$$X)
> +endef
> +$(eval $(call bin_wrappers_templates))
> +
> +all:: $(BIN_WRAPPERS)

... what it expands to looked quite straight-forward.