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

On 28/02/2022 11:16, Ævar Arnfjörð Bjarmason wrote:

On Mon, Feb 28 2022, Phillip Wood wrote:

quoted

Hi Ævar

On 25/02/2022 09:04, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

quoted

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.

quoted

quoted

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