| Message ID | patch-v3-2.9-96a490bec54-20220225T090127Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Makefile: optimize noop runs, add shared.mak | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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:: Continuing with the theme of [1/9], this change to Makefile gets my firm NAK. The first two lines MUST stay to be the first lines. Otherwise, a mistaken patch that adds rules to shared.make will make the first of these rules, not "all", as the default target. Just move it below to the third line or so and you'd be OK.
On Fri, Feb 25 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 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:: > > Continuing with the theme of [1/9], this change to Makefile gets my > firm NAK. The first two lines MUST stay to be the first lines. > > Otherwise, a mistaken patch that adds rules to shared.make will > make the first of these rules, not "all", as the default target. > > Just move it below to the third line or so and you'd be OK. Understood, I can/will do that. But wouldn't working this into this series be an even better option? The ".DEFAULT_GOAL" variable is new in, 3.81 (01 Apr 2006), which I think is old enough to hard depend on. We use $(or ...) added in the same release in config.mak.dev. We definitely have a hard dependency on "Version 3.80 (03 Oct 2002)" $(eval ...) etc. I checked and RHEL5 (which is otherwise the oldest OS we've supported) has it, but RHEL4 doesn't (it has 3.80). So I think it's safe to have a hard dependency on it and other things in 3.81 at this point. diff --git a/Makefile b/Makefile index 549ca6e7a5c..03321af7e3e 100644 --- a/Makefile +++ b/Makefile @@ -1,9 +1,6 @@ # Import tree-wide shared Makefile behavior and libraries include shared.mak -# The default target of this Makefile is... -all:: - # Define V=1 to have a more verbose compile. # # Define SHELL_PATH to a POSIX shell if your /bin/sh is broken. diff --git a/shared.mak b/shared.mak index 7ba04049c8a..1034dc758f1 100644 --- a/shared.mak +++ b/shared.mak @@ -1,3 +1,8 @@ +### By setting the default goal to "all" we override any implicit +### setting once "make" sees our first target, which it'll ignore if +### .DEFAULT_GOAL was explicitly set. +.DEFAULT_GOAL = all + ### Remove GNU make implicit rules ## This speeds things up since we don't need to look for and stat() a
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But wouldn't working this into this series be an even better option? No. What our current Makefile does (i.e. to have the default goal at the beginning, even though the recipe for it may live elsewhere) is a very well understood and established pattern, and there is no reason to start doing the same thing in a different way, even if it is safe to assume that the "different way" may be available everywhere. We are not gaining anything from doing it differently. Especially it is not something I want to see as "while at it" change, which adds more to the review cycle without improving anything. > The ".DEFAULT_GOAL" variable is new in, 3.81 (01 Apr 2006), which I > think is old enough to hard depend on. We use $(or ...) added in the > same release in config.mak.dev.
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. Also does this patch mean we're now using .DELETE_ON_ERROR in places where we were not previously using it? Best Wishes Phillip > 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
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. 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
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 >
On Mon, Feb 28 2022, Phillip Wood wrote: > 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. Thanks, yes, willdo :) I have a re-roll of this queued up for submission, after sitting on it for a bit longer to shake out any potential last-minute issues...
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
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. 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