From: Phillip Wood Date: Mon, 28 Feb 2022 15:51:27 GMT Subject: Re: [PATCH v3 2/9] Makefiles: add "shared.mak", move ".DELETE_ON_ERROR" to it Message-ID: <90d23574-a211-1294-c842-9e033757972d@gmail.com> In-Reply-To: <220228.867d9f5jat.gmgdl@evledraar.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 >>> --- >>> 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 >