Re: [PATCH v3 2/9] Makefiles: add "shared.mak", move ".DELETE_ON_ERROR" to it

[Phillip Wood <phillip.wood123@gmail.com>]

On 28/02/2022 11:16, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Feb 28 2022, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 25/02/2022 09:04, Ævar Arnfjörð Bjarmason wrote:
>>> We have various behavior that's shared across our Makefiles, or that
>>> really should be (e.g. via defined templates). Let's create a
>>> top-level "shared.mak" to house those sorts of things, and start by
>>> adding the ".DELETE_ON_ERROR" flag to it.
>>> See my own 7b76d6bf221 (Makefile: add and use the ".DELETE_ON_ERROR"
>>> flag, 2021-06-29) and db10fc6c09f (doc: simplify Makefile using
>>> .DELETE_ON_ERROR, 2021-05-21) for the addition and use of the
>>> ".DELETE_ON_ERROR" flag.
>>> This does have the potential downside that if
>>> e.g. templates/Makefile
>>> would like to include this "shared.mak" in the future the semantics of
>>> such a Makefile will change, but as noted in the above commits (and
>>> GNU make's own documentation) any such change would be for the better,
>>> so it's safe to do this.
>>
>> I was confused about the mention of templates/Makefile in this
>> paragraph, it seems to be saying that the behavior would change in the
>> future if we included shared.mak in templates/Makefile but this patch
>> does exactly that.
> 
> Yes, oops! It's a zombie comment that I forgot to adjust from an earlier
> version of this where that wasn't the case. Will adjust & re-roll.
> 
>> Also does this patch mean we're now using .DELETE_ON_ERROR in places
>> where we were not previously using it?
> 
> Yes, we'll now use it in those other Makefiles as well. The commits
> referenced in the second paragrap of the commit message argue for this
> being safe, and I've reviewed the logic myself & don't expect any
> problems with it.

Thanks for elaborating, maybe it is worth spelling explicitly in the 
commit message that this is turning on .DELETE_ON_ERROR in places we 
didn't previously use it. I had a look at the commit message you 
referenced and it seems to make a good case for using .DELETE_ON_ERROR. 
Having a shared makefile for common code makes sense and the speed ups 
from some of the other commits are nice.

Best Wishes

Phillip

> As the GNU make manual itself notes the cases where that would cause
> problems are really obscure, but it's not the default out of an
> abundance of backwards compatibility caution in GNU make.
> 
>>> This also doesn't introduce a bug by e.g. having this
>>> ".DELETE_ON_ERROR" flag only apply to this new shared.mak, Makefiles
>>> have no such scoping semantics.
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>    Documentation/Makefile    |  6 +++---
>>>    Makefile                  | 13 +++----------
>>>    contrib/scalar/Makefile   |  3 +++
>>>    contrib/scalar/t/Makefile |  3 +++
>>>    shared.mak                |  9 +++++++++
>>>    t/Makefile                |  3 +++
>>>    t/interop/Makefile        |  3 +++
>>>    t/perf/Makefile           |  3 +++
>>>    templates/Makefile        |  3 +++
>>>    9 files changed, 33 insertions(+), 13 deletions(-)
>>>    create mode 100644 shared.mak
>>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>>> index ed656db2ae9..ba27456c86a 100644
>>> --- a/Documentation/Makefile
>>> +++ b/Documentation/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../shared.mak
>>> +
>>>    # Guard against environment variables
>>>    MAN1_TXT =
>>>    MAN5_TXT =
>>> @@ -524,7 +527,4 @@ doc-l10n install-l10n::
>>>    	$(MAKE) -C po $@
>>>    endif
>>>    -# Delete the target file on error
>>> -.DELETE_ON_ERROR:
>>> -
>>>    .PHONY: FORCE
>>> diff --git a/Makefile b/Makefile
>>> index 6f0b4b775fe..d378ec22545 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include shared.mak
>>> +
>>>    # The default target of this Makefile is...
>>>    all::
>>>    @@ -2194,16 +2197,6 @@ shell_compatibility_test:
>>> please_set_SHELL_PATH_to_a_more_modern_shell
>>>    strip: $(PROGRAMS) git$X
>>>    	$(STRIP) $(STRIP_OPTS) $^
>>>    -### Flags affecting all rules
>>> -
>>> -# A GNU make extension since gmake 3.72 (released in late 1994) to
>>> -# remove the target of rules if commands in those rules fail. The
>>> -# default is to only do that if make itself receives a signal. Affects
>>> -# all targets, see:
>>> -#
>>> -#    info make --index-search=.DELETE_ON_ERROR
>>> -.DELETE_ON_ERROR:
>>> -
>>>    ### Target-specific flags and dependencies
>>>      # The generic compilation pattern rule and automatically
>>> diff --git a/contrib/scalar/Makefile b/contrib/scalar/Makefile
>>> index 5b12a437426..6fb5cc8b701 100644
>>> --- a/contrib/scalar/Makefile
>>> +++ b/contrib/scalar/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../../shared.mak
>>> +
>>>    QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
>>>    QUIET_SUBDIR1  =
>>>    diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile
>>> index 6170672bb37..01e82e56d15 100644
>>> --- a/contrib/scalar/t/Makefile
>>> +++ b/contrib/scalar/t/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../../../shared.mak
>>> +
>>>    # Run scalar tests
>>>    #
>>>    # Copyright (c) 2005,2021 Junio C Hamano, Johannes Schindelin
>>> diff --git a/shared.mak b/shared.mak
>>> new file mode 100644
>>> index 00000000000..0170bb397ae
>>> --- /dev/null
>>> +++ b/shared.mak
>>> @@ -0,0 +1,9 @@
>>> +### Flags affecting all rules
>>> +
>>> +# A GNU make extension since gmake 3.72 (released in late 1994) to
>>> +# remove the target of rules if commands in those rules fail. The
>>> +# default is to only do that if make itself receives a signal. Affects
>>> +# all targets, see:
>>> +#
>>> +#    info make --index-search=.DELETE_ON_ERROR
>>> +.DELETE_ON_ERROR:
>>> diff --git a/t/Makefile b/t/Makefile
>>> index 46cd5fc5273..056ce55dcc9 100644
>>> --- a/t/Makefile
>>> +++ b/t/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../shared.mak
>>> +
>>>    # Run tests
>>>    #
>>>    # Copyright (c) 2005 Junio C Hamano
>>> diff --git a/t/interop/Makefile b/t/interop/Makefile
>>> index 31a4bbc716a..6911c2915a7 100644
>>> --- a/t/interop/Makefile
>>> +++ b/t/interop/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../../shared.mak
>>> +
>>>    -include ../../config.mak
>>>    export GIT_TEST_OPTIONS
>>>    diff --git a/t/perf/Makefile b/t/perf/Makefile
>>> index 2465770a782..e4808aebed0 100644
>>> --- a/t/perf/Makefile
>>> +++ b/t/perf/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../../shared.mak
>>> +
>>>    -include ../../config.mak
>>>    export GIT_TEST_OPTIONS
>>>    diff --git a/templates/Makefile b/templates/Makefile
>>> index d22a71a3999..636cee52f51 100644
>>> --- a/templates/Makefile
>>> +++ b/templates/Makefile
>>> @@ -1,3 +1,6 @@
>>> +# Import tree-wide shared Makefile behavior and libraries
>>> +include ../shared.mak
>>> +
>>>    # make and install sample templates
>>>      ifndef V
>