[PATCH 0/1] Avoid multiple patterns when recipes generate one file

git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed

* [PATCH 0/1] Avoid multiple patterns when recipes generate one file
@ 2022-11-27 22:42 Paul Smith
  2022-11-27 22:42 ` [PATCH 1/1] " Paul Smith
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 23+ messages in thread
From: Paul Smith @ 2022-11-27 22:42 UTC (permalink / raw)
  To: git; +Cc: avarab

Needed for GNU make 4.4 or above.  This is backward-compatible with
all previous versions of GNU make.

I'm not sure if this should be considered a bug fix; I based it on
maint just in case.

Paul Smith (1):
  Avoid multiple patterns when recipes generate one file

 Documentation/Makefile | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)


base-commit: e7e5c6f715b2de7bea0d39c7d2ba887335b40aa0
-- 
2.35.3


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/1] Avoid multiple patterns when recipes generate one file
  2022-11-27 22:42 [PATCH 0/1] Avoid multiple patterns when recipes generate one file Paul Smith
@ 2022-11-27 22:42 ` Paul Smith
  2022-11-28 13:08   ` Ævar Arnfjörð Bjarmason
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 23+ messages in thread
From: Paul Smith @ 2022-11-27 22:42 UTC (permalink / raw)
  To: git; +Cc: avarab, Alexander Kanavin

A GNU make pattern rule with multiple targets has always meant that
a single invocation of the recipe will build all the targets.
However in older versions of GNU make a recipe that did not really
build all the targets would be tolerated.

Starting with GNU make 4.4 this behavior is deprecated and pattern
rules are expected to generate files to match all the patterns.
If not all targets are created then GNU make will not consider any
target up to date and will re-run the recipe when it is run again.

Modify Documentation/Makefile to split the man page-creating pattern
rule into a separate pattern rule for each pattern.

Reported-by: Alexander Kanavin <alex.kanavin@gmail.com>
Signed-off-by: Paul Smith <psmith@gnu.org>
---
 Documentation/Makefile | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d47acb2e25..21375cd3f2 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -351,8 +351,16 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
-	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+
+manpage-prereqs := manpage-base-url.xsl $(wildcard manpage*.xsl)
+manpage-cmd = $(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+
+%.1 : %.xml $(manpage-prereqs)
+	$(manpage-cmd)
+%.5 : %.xml $(manpage-prereqs)
+	$(manpage-cmd)
+%.7 : %.xml $(manpage-prereqs)
+	$(manpage-cmd)
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/1] Avoid multiple patterns when recipes generate one file
  2022-11-27 22:42 ` [PATCH 1/1] " Paul Smith
@ 2022-11-28 13:08   ` Ævar Arnfjörð Bjarmason
  2022-11-28 18:33     ` Paul Smith
  0 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-28 13:08 UTC (permalink / raw)
  To: Paul Smith; +Cc: git, Alexander Kanavin


On Sun, Nov 27 2022, Paul Smith wrote:

> A GNU make pattern rule with multiple targets has always meant that
> a single invocation of the recipe will build all the targets.
> However in older versions of GNU make a recipe that did not really
> build all the targets would be tolerated.
>
> Starting with GNU make 4.4 this behavior is deprecated and pattern
> rules are expected to generate files to match all the patterns.
> If not all targets are created then GNU make will not consider any
> target up to date and will re-run the recipe when it is run again.
>
> Modify Documentation/Makefile to split the man page-creating pattern
> rule into a separate pattern rule for each pattern.
>
> Reported-by: Alexander Kanavin <alex.kanavin@gmail.com>
> Signed-off-by: Paul Smith <psmith@gnu.org>
> ---

Thanks for fixing downstream, and for working on GNU make.

>  Documentation/Makefile | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index d47acb2e25..21375cd3f2 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -351,8 +351,16 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
>  manpage-base-url.xsl: manpage-base-url.xsl.in
>  	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
>  
> -%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
> -	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
> +
> +manpage-prereqs := manpage-base-url.xsl $(wildcard manpage*.xsl)
> +manpage-cmd = $(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
> +
> +%.1 : %.xml $(manpage-prereqs)
> +	$(manpage-cmd)
> +%.5 : %.xml $(manpage-prereqs)
> +	$(manpage-cmd)
> +%.7 : %.xml $(manpage-prereqs)
> +	$(manpage-cmd)
>  
>  %.xml : %.txt $(ASCIIDOC_DEPS)
>  	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<

Whether we use eval/define or not (I just tried to avoid the repetition)
I think referring to $(DOC_MAN[157]) here probably makes more sense if
we're poking at these rules.

I.e. in this case the rest of the Makefile is carrying forward what
manpages we're generating exactly, so rather than a wildcard %.1 to
%.xml we can narrow it down to just the %.1 files we're going to b
generating (but maybe that's best left for later...):

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 5e1a7f655c2..7404cead084 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -351,8 +351,12 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
-	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+define doc-man-tmpl
+$$(DOC_MAN$(1)): %.$(1) : %.xml manpage-base-url.xsl $$(wildcard manpage*.xsl)
+	$$(QUIET_XMLTO)$$(XMLTO) -m $$(MANPAGE_XSL) $$(XMLTO_EXTRA) man $$<
+
+endef
+$(eval $(foreach n,1 5 7,$(call doc-man-tmpl,$(n))))
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/1] Avoid multiple patterns when recipes generate one file
  2022-11-28 13:08   ` Ævar Arnfjörð Bjarmason
@ 2022-11-28 18:33     ` Paul Smith
  2022-11-28 18:57       ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 23+ messages in thread
From: Paul Smith @ 2022-11-28 18:33 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Alexander Kanavin

On Mon, 2022-11-28 at 14:08 +0100, Ævar Arnfjörð Bjarmason wrote:
> Whether we use eval/define or not (I just tried to avoid the
> repetition) I think referring to $(DOC_MAN[157]) here probably makes
> more sense if we're poking at these rules.
> 
> I.e. in this case the rest of the Makefile is carrying forward what
> manpages we're generating exactly, so rather than a wildcard %.1 to
> %.xml we can narrow it down to just the %.1 files we're going to b
> generating (but maybe that's best left for later...):

I have no opinion on which is better :).

I'm not sure what the above comment is asking for though: are you going
to take over pushing this change?  Or do you want me to reroll the
commit with these changes instead?  Or are we waiting for more
opinions?

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 5e1a7f655c2..7404cead084 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -351,8 +351,12 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
>  manpage-base-url.xsl: manpage-base-url.xsl.in
>         $(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
>  
> -%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
> -       $(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
> +define doc-man-tmpl
> +$$(DOC_MAN$(1)): %.$(1) : %.xml manpage-base-url.xsl $$(wildcard manpage*.xsl)
> +       $$(QUIET_XMLTO)$$(XMLTO) -m $$(MANPAGE_XSL) $$(XMLTO_EXTRA) man $$<
> +
> +endef
> +$(eval $(foreach n,1 5 7,$(call doc-man-tmpl,$(n))))
>  
>  %.xml : %.txt $(ASCIIDOC_DEPS)
>         $(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/1] Avoid multiple patterns when recipes generate one file
  2022-11-28 18:33     ` Paul Smith
@ 2022-11-28 18:57       ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-28 18:57 UTC (permalink / raw)
  To: psmith; +Cc: git, Alexander Kanavin


On Mon, Nov 28 2022, Paul Smith wrote:

> On Mon, 2022-11-28 at 14:08 +0100, Ævar Arnfjörð Bjarmason wrote:
>> Whether we use eval/define or not (I just tried to avoid the
>> repetition) I think referring to $(DOC_MAN[157]) here probably makes
>> more sense if we're poking at these rules.
>> 
>> I.e. in this case the rest of the Makefile is carrying forward what
>> manpages we're generating exactly, so rather than a wildcard %.1 to
>> %.xml we can narrow it down to just the %.1 files we're going to b
>> generating (but maybe that's best left for later...):
>
> I have no opinion on which is better :).
>
> I'm not sure what the above comment is asking for though: are you going
> to take over pushing this change?  Or do you want me to reroll the
> commit with these changes instead?  Or are we waiting for more
> opinions?

Just a suggestion in case you thought it helped, but I think we can just
go for your version.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes
  2022-11-27 22:42 [PATCH 0/1] Avoid multiple patterns when recipes generate one file Paul Smith
  2022-11-27 22:42 ` [PATCH 1/1] " Paul Smith
@ 2022-11-29 14:09 ` Ævar Arnfjörð Bjarmason
  2022-11-29 14:09   ` [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list Ævar Arnfjörð Bjarmason
                     ` (5 more replies)
  1 sibling, 6 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-29 14:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

GNU Make 4.4 was released just about a month ago[1], this series picks
up & amends a change from Paul Smith (the GNU make maintainer), and
then fixes another bug in our Makefiles as a result of a
backwards-incompatible change in how $(MAKEFLAGS) works in 4.4.

Junio: I think this is worth considering for merging down in the rc
peried. We can limp along without these fixes, but not being able to
build the docs to completion (as far as make is concerned) and the new
warnings fixed by 2/4 will probably break things for or annoy some
packagers.

The 3/4 then fixes the output being always-verbose for our
sub-Makefiles for the affected targets. 4/4 is pure-refactoring, but I
think should help build confidence in the preceding changes.

1. https://lwn.net/Articles/913253/

Paul Smith (1):
  Documentation/Makefile: avoid multiple patterns when generating one
    file

Ævar Arnfjörð Bjarmason (3):
  Documentation/Makefile: de-duplicate *.[157] dependency list
  Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  Documentation/Makefile: narrow wildcard rules to our known files

 Documentation/Makefile | 15 ++++++++++++---
 git-gui/Makefile       |  2 +-
 shared.mak             |  4 ++--
 3 files changed, 15 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  115d79fe1fc ! 1:  42b4f241c97 Avoid multiple patterns when recipes generate one file
    @@
      ## Metadata ##
    -Author: Paul Smith <psmith@gnu.org>
    +Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Avoid multiple patterns when recipes generate one file
    +    Documentation/Makefile: de-duplicate *.[157] dependency list
     
    -    A GNU make pattern rule with multiple targets has always meant that
    -    a single invocation of the recipe will build all the targets.
    -    However in older versions of GNU make a recipe that did not really
    -    build all the targets would be tolerated.
    +    Use the "DOC_MAN[157]" variables combined into a new "DOC_MANN" to
    +    declare that e.g. "git-am.1" depends on "manpage-base-url.xsl"
    +    etc. This change helps to make a subsequent change smaller.
     
    -    Starting with GNU make 4.4 this behavior is deprecated and pattern
    -    rules are expected to generate files to match all the patterns.
    -    If not all targets are created then GNU make will not consider any
    -    target up to date and will re-run the recipe when it is run again.
    -
    -    Modify Documentation/Makefile to split the man page-creating pattern
    -    rule into a separate pattern rule for each pattern.
    -
    -    Reported-by: Alexander Kanavin <alex.kanavin@gmail.com>
    -    Signed-off-by: Paul Smith <psmith@gnu.org>
    +    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/Makefile ##
    +@@ Documentation/Makefile: ARTICLES_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
    + HTML_FILTER ?= $(ARTICLES_HTML) $(OBSOLETE_HTML)
    + DOC_HTML = $(MAN_HTML) $(filter $(HTML_FILTER),$(ARTICLES_HTML) $(OBSOLETE_HTML))
    + 
    ++DOC_MANN =
    + DOC_MAN1 = $(patsubst %.txt,%.1,$(filter $(MAN_FILTER),$(MAN1_TXT)))
    ++DOC_MANN += $(DOC_MAN1)
    + DOC_MAN5 = $(patsubst %.txt,%.5,$(filter $(MAN_FILTER),$(MAN5_TXT)))
    ++DOC_MANN += $(DOC_MAN5)
    + DOC_MAN7 = $(patsubst %.txt,%.7,$(filter $(MAN_FILTER),$(MAN7_TXT)))
    ++DOC_MANN += $(DOC_MAN7)
    + 
    + prefix ?= $(HOME)
    + bindir ?= $(prefix)/bin
     @@ Documentation/Makefile: $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
      manpage-base-url.xsl: manpage-base-url.xsl.in
      	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
      
     -%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
    --	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
    -+
    -+manpage-prereqs := manpage-base-url.xsl $(wildcard manpage*.xsl)
    -+manpage-cmd = $(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
    -+
    -+%.1 : %.xml $(manpage-prereqs)
    -+	$(manpage-cmd)
    -+%.5 : %.xml $(manpage-prereqs)
    -+	$(manpage-cmd)
    -+%.7 : %.xml $(manpage-prereqs)
    -+	$(manpage-cmd)
    ++$(DOC_MANN): manpage-base-url.xsl $(wildcard manpage*.xsl)
    ++%.1 %.5 %.7 : %.xml
    + 	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
      
      %.xml : %.txt $(ASCIIDOC_DEPS)
    - 	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<
-:  ----------- > 2:  e232f308e40 Documentation/Makefile: avoid multiple patterns when generating one file
-:  ----------- > 3:  6db7dd74e52 Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
-:  ----------- > 4:  f1bc3c16904 Documentation/Makefile: narrow wildcard rules to our known files
-- 
2.39.0.rc0.993.g0c499e58e3b


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
@ 2022-11-29 14:09   ` Ævar Arnfjörð Bjarmason
  2022-11-30  4:17     ` Junio C Hamano
  2022-11-29 14:09   ` [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-29 14:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

Use the "DOC_MAN[157]" variables combined into a new "DOC_MANN" to
declare that e.g. "git-am.1" depends on "manpage-base-url.xsl"
etc. This change helps to make a subsequent change smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 5e1a7f655c2..d239f6751f0 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -129,9 +129,13 @@ ARTICLES_HTML += $(patsubst %,%.html,$(ARTICLES) $(SP_ARTICLES))
 HTML_FILTER ?= $(ARTICLES_HTML) $(OBSOLETE_HTML)
 DOC_HTML = $(MAN_HTML) $(filter $(HTML_FILTER),$(ARTICLES_HTML) $(OBSOLETE_HTML))
 
+DOC_MANN =
 DOC_MAN1 = $(patsubst %.txt,%.1,$(filter $(MAN_FILTER),$(MAN1_TXT)))
+DOC_MANN += $(DOC_MAN1)
 DOC_MAN5 = $(patsubst %.txt,%.5,$(filter $(MAN_FILTER),$(MAN5_TXT)))
+DOC_MANN += $(DOC_MAN5)
 DOC_MAN7 = $(patsubst %.txt,%.7,$(filter $(MAN_FILTER),$(MAN7_TXT)))
+DOC_MANN += $(DOC_MAN7)
 
 prefix ?= $(HOME)
 bindir ?= $(prefix)/bin
@@ -351,7 +355,8 @@ $(OBSOLETE_HTML): %.html : %.txto $(ASCIIDOC_DEPS)
 manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
-%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
+$(DOC_MANN): manpage-base-url.xsl $(wildcard manpage*.xsl)
+%.1 %.5 %.7 : %.xml
 	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
-- 
2.39.0.rc0.993.g0c499e58e3b


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list
  2022-11-29 14:09   ` [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list Ævar Arnfjörð Bjarmason
@ 2022-11-30  4:17     ` Junio C Hamano
  0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2022-11-30  4:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Paul Smith

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

> -%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
> +$(DOC_MANN): manpage-base-url.xsl $(wildcard manpage*.xsl)

Not a new issue, but to avoid getting affected by an untracked new
xsl files, shouldn't we expand the wildcard at the source level
here?  I.e.

    $(DOC_MANN): manpage-base-url.xsl \
            manpage-bold-literal.xsl \
            manpage-normal.xsl \
            manpage-quote-apos.xsl \
            manpage.xsl

or something?

> +%.1 %.5 %.7 : %.xml
>  	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
>  
>  %.xml : %.txt $(ASCIIDOC_DEPS)

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
  2022-11-29 14:09   ` [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list Ævar Arnfjörð Bjarmason
@ 2022-11-29 14:09   ` Ævar Arnfjörð Bjarmason
  2022-11-30  4:18     ` Junio C Hamano
  2022-11-29 14:09   ` [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-29 14:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Paul Smith, Alexander Kanavin,
	Ævar Arnfjörð Bjarmason

From: Paul Smith <psmith@gnu.org>

A GNU make pattern rule with multiple targets has always meant that
a single invocation of the recipe will build all the targets.
However in older versions of GNU make a recipe that did not really
build all the targets would be tolerated.

Starting with GNU make 4.4 this behavior is deprecated and pattern
rules are expected to generate files to match all the patterns.
If not all targets are created then GNU make will not consider any
target up to date and will re-run the recipe when it is run again.

I.e. a command like:

	make -C Documentation git-am.1

Will never be satisfied that "git-am.1" has been made, because we
didn't also make "git-am.5" and "git-am.7", as the warning it'll emit
indicates:

	$ make -C Documentation git-am.1
	[...]
	    XMLTO git-am.1
	Makefile:355: warning: pattern recipe did not update peer target 'git-am.7'.
	Makefile:355: warning: pattern recipe did not update peer target 'git-am.5'.

Modify Documentation/Makefile to split the man page-creating pattern
rule into a separate pattern rule for each pattern. This requires a
small amount of copy/pasting, but due to splitting out the "DOC_MANN"
in the preceding commit it's not too bad.

Reported-by: Alexander Kanavin <alex.kanavin@gmail.com>
Signed-off-by: Paul Smith <psmith@gnu.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/Makefile | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index d239f6751f0..89929e3d60b 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -356,7 +356,11 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
 $(DOC_MANN): manpage-base-url.xsl $(wildcard manpage*.xsl)
-%.1 %.5 %.7 : %.xml
+%.1 : %.xml
+	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+%.5 : %.xml
+	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
+%.7 : %.xml
 	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
 
 %.xml : %.txt $(ASCIIDOC_DEPS)
-- 
2.39.0.rc0.993.g0c499e58e3b


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file
  2022-11-29 14:09   ` [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file Ævar Arnfjörð Bjarmason
@ 2022-11-30  4:18     ` Junio C Hamano
  0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2022-11-30  4:18 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Paul Smith, Alexander Kanavin

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

> From: Paul Smith <psmith@gnu.org>
>
> A GNU make pattern rule with multiple targets has always meant that
> a single invocation of the recipe will build all the targets.
> However in older versions of GNU make a recipe that did not really
> build all the targets would be tolerated.

This was in 'next' and was merged to -rc1 already.

Thanks, both.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
  2022-11-29 14:09   ` [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list Ævar Arnfjörð Bjarmason
  2022-11-29 14:09   ` [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file Ævar Arnfjörð Bjarmason
@ 2022-11-29 14:09   ` Ævar Arnfjörð Bjarmason
  2022-11-30  4:28     ` Junio C Hamano
  2022-11-29 14:09   ` [PATCH v2 4/4] Documentation/Makefile: narrow wildcard rules to our known files Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-29 14:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
changed in a backward-incompatible way, as its "NEWS" file notes:

  Previously only simple (one-letter) options were added to the MAKEFLAGS
  variable that was visible while parsing makefiles.  Now, all options are
  available in MAKEFLAGS.  If you want to check MAKEFLAGS for a one-letter
  option, expanding "$(firstword -$(MAKEFLAGS))" is a reliable way to return
  the set of one-letter options which can be examined via findstring, etc.

This upstream change meant that e.g.:

	make man

Would become very noisy, because in shared.mak we rely on extracting
"s" from the $(MAKEFLAGS), which now contains long options like
"--jobserver-auth=fifo:<path>", which we'll conflate with the "-s"
option.

So, let's change this idiom we've been carrying since [1], [2] and [3]
as the "NEWS" suggests.

Note that the "-" in "-$(MAKEFLAGS)" is critical here, as the variable
will always contain leading whitespace if there are no short options,
but long options are present. Without it e.g. "make --debug=all" would
yield "--debug=all" as the first word, but with it we'll get "-" as
intended. Then "-s" for "-s", "-Bs" for "-s -B" etc.

1. 0c3b4aac8ec (git-gui: Support of "make -s" in: do not output
   anything of the build itself, 2007-03-07)
2. b777434383b (Support of "make -s": do not output anything of the
   build itself, 2007-03-07)
3. bb2300976ba (Documentation/Makefile: make most operations "quiet",
   2009-03-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-gui/Makefile | 2 +-
 shared.mak       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-gui/Makefile b/git-gui/Makefile
index 56c85a85c1e..a0d5a4b28e1 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -116,7 +116,7 @@ ifeq ($(uname_S),Darwin)
 	TKEXECUTABLE = $(shell basename "$(TKFRAMEWORK)" .app)
 endif
 
-ifeq ($(findstring $(MAKEFLAGS),s),s)
+ifeq ($(findstring $(firstword -$(MAKEFLAGS)),s),s)
 QUIET_GEN =
 endif
 
diff --git a/shared.mak b/shared.mak
index be1f30ff206..aeb80fc4d5a 100644
--- a/shared.mak
+++ b/shared.mak
@@ -37,13 +37,13 @@ space := $(empty) $(empty)
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
-ifneq ($(findstring w,$(MAKEFLAGS)),w)
+ifneq ($(findstring w,$(firstword -$(MAKEFLAGS))),w)
 PRINT_DIR = --no-print-directory
 else # "make -w"
 NO_SUBDIR = :
 endif
 
-ifneq ($(findstring s,$(MAKEFLAGS)),s)
+ifneq ($(findstring s,$(firstword -$(MAKEFLAGS))),s)
 ifndef V
 ## common
 	QUIET_SUBDIR0  = +@subdir=
-- 
2.39.0.rc0.993.g0c499e58e3b


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-29 14:09   ` [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
@ 2022-11-30  4:28     ` Junio C Hamano
  2022-11-30  5:49       ` Paul Smith
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2022-11-30  4:28 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Paul Smith

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

> Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
> changed in a backward-incompatible way, as its "NEWS" file notes:
>
>   Previously only simple (one-letter) options were added to the MAKEFLAGS
>   variable that was visible while parsing makefiles.  Now, all options are
>   available in MAKEFLAGS.  If you want to check MAKEFLAGS for a one-letter
>   option, expanding "$(firstword -$(MAKEFLAGS))" is a reliable way to return
>   the set of one-letter options which can be examined via findstring, etc.

Wow.  That's a bold move for GNU make folks to make.

> This upstream change meant that e.g.:
>
> 	make man
>
> Would become very noisy, because in shared.mak we rely on extracting
> "s" from the $(MAKEFLAGS), which now contains long options like
> "--jobserver-auth=fifo:<path>", which we'll conflate with the "-s"
> option.

Do our uses of $(MAKEFLAGS) for the $(PRINT_DIR) and the $(QUIET)
macros that do not affect correctness?  $(QUIET) thing I suspect
will merely be annoyance, but $(PRINT_DIR) might affect correctness
depending on how $(MAKE) output is being used.

I have to wonder how many projects they have broken with this change
;-).

In any case, this seems like a good thing to do.  I am not sure if
this is so urgent to add in the -rc period, or can safely wait post
release.

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30  4:28     ` Junio C Hamano
@ 2022-11-30  5:49       ` Paul Smith
  2022-12-01 12:37         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 23+ messages in thread
From: Paul Smith @ 2022-11-30  5:49 UTC (permalink / raw)
  To: git

On Wed, 2022-11-30 at 13:28 +0900, Junio C Hamano wrote:
> I have to wonder how many projects they have broken with this change

Some, but not that many.  Most projects don't try to investigate
MAKEFLAGS, and of those that do many were already using the recommended
method, because even prior to GNU make 4.4 it was possible for
MAKEFLAGS to have stray "s" characters, in unusual situations (for
example if MAKEFLAGS were set in the makefile).

There were various bugs filed that various options could not be
investigated from within makefiles and also that running make from
within $(shell ...) didn't work right because MAKEFLAGS was not set.

It was just a mess, trying to keep the value of MAKEFLAGS set to
different values at different points in the processing of make.

Also, ensuring this trick for searching MAKEFLAGS continues to work
would have meant strictly controlling what new options we could add to
GNU make.  I haven't seen any other project use the filter-out --%
trick that the Git makefiles do, but even with that it won't help if a
new single-letter option that takes an argument is added.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30  5:49       ` Paul Smith
@ 2022-12-01 12:37         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-01 12:37 UTC (permalink / raw)
  To: psmith; +Cc: git


On Wed, Nov 30 2022, Paul Smith wrote:

> On Wed, 2022-11-30 at 13:28 +0900, Junio C Hamano wrote:
>> I have to wonder how many projects they have broken with this change
>
> Some, but not that many.  Most projects don't try to investigate
> MAKEFLAGS, and of those that do many were already using the recommended
> method, because even prior to GNU make 4.4 it was possible for
> MAKEFLAGS to have stray "s" characters, in unusual situations (for
> example if MAKEFLAGS were set in the makefile).
>
> There were various bugs filed that various options could not be
> investigated from within makefiles and also that running make from
> within $(shell ...) didn't work right because MAKEFLAGS was not set.
>
> It was just a mess, trying to keep the value of MAKEFLAGS set to
> different values at different points in the processing of make.

It was definitely a bit of a hack on our part, but to be fair before
this 4.4 release doing it this way was recommended by the
documentation. I see you changed that recently, but maybe this on top
makes sense?
	
	diff --git a/doc/make.texi b/doc/make.texi
	index e3a3ade4..9e9a894e 100644
	--- a/doc/make.texi
	+++ b/doc/make.texi
	@@ -5069,7 +5069,7 @@ Variable @code{MAKEFILES}}.
	 @vindex MAKEFLAGS
	 Flags such as @samp{-s} and @samp{-k} are passed automatically to the
	 sub-@code{make} through the variable @code{MAKEFLAGS}.  This variable is
	-set up automatically by @code{make} to contain the flag letters that
	+set up automatically by @code{make} to contain the normalized flag letters that
	 @code{make} received.  Thus, if you do @w{@samp{make -ks}} then
	 @code{MAKEFLAGS} gets the value @samp{ks}.
	 
	@@ -5085,6 +5085,10 @@ option has both single-letter and long options, the single-letter option is
	 always preferred.  If there are no single-letter options on the command line,
	 then the value of @code{MAKEFLAGS} starts with a space.
	 
	+The value of @code{MAKEFLAGS} does not correspond to the order in which
	+command line options are provided. Both @w{@samp{make -sk}} and @w{@samp{make -sk}}
	+will result in a @code{MAKEFLAGS} value of @samp{ks}.
	+
	 @cindex command line variable definitions, and recursion
	 @cindex variables, command line, and recursion
	 @cindex recursion, and command line variable definitions
	@@ -12378,10 +12382,13 @@ influences such as interrupts (@code{SIGINT}), etc.  You may want to install
	 signal handlers to manage this write-back.
	 
	 @item
	-Your tool may also examine the first word of the @code{MAKEFLAGS} variable and
	+Your tool may also examine the first word of the @samp{-$(MAKEFLAGS)} expression and
	 look for the character @code{n}.  If this character is present then
	 @code{make} was invoked with the @samp{-n} option and your tool may want to
	 stop without performing any operations.
	+
	+Note that this is not equivalent to checking for the first word of
	+@code{MAKEFLAGS}.
	 @end itemize
	 
	 @node Windows Jobserver,  , POSIX Jobserver, Job Slots

I.e. that "Your tool" part seems to still be assuming 4.3 semantics.

> Also, ensuring this trick for searching MAKEFLAGS continues to work
> would have meant strictly controlling what new options we could add to
> GNU make.  I haven't seen any other project use the filter-out --%
> trick that the Git makefiles do, but even with that it won't help if a
> new single-letter option that takes an argument is added.

I'd think it would probably make sense to promise that GNU make will
never add such options, so that what's currently documented continues to
work. I.e. it supports:

	--debug=all

Not these forms:

	-dwhy
	-d=why

But if we're on the topic: The only reason git's Makefile uses these is
because it's trying to fake up some pretty verbose-but-not-too-verbose
mode. You can see this in our tree at "shared.mak", the kernel does
something similar.

For our Makefile this is pretty close to what we'd get from a simpler:

	make -B --debug=why -s |
        sed -E -n \
		-e "s/.*: update target '(.*).o' due to.*/   CC \\1.o/" \
                -e 's/due to.*//' \
                -e 'p'

I.e. when we have %.o" targets this is emitting " CC $@" lines, I've
left matching the rest as an excercise for the reader, but it would be
e.g. "GEN_PERL" or whatever for %.pm and so on.

I don't know what this would look like exactly, but it would be neat if
GNU make supported some way to emit such friendly output in
general. Something like a sprintf format where you'd have access to the
sort of input that "due to" string gets internally (and perhaps a bit
more, e.g. something indicating overall progress through the graph...).


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 4/4] Documentation/Makefile: narrow wildcard rules to our known files
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2022-11-29 14:09   ` [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
@ 2022-11-29 14:09   ` Ævar Arnfjörð Bjarmason
  2022-11-30  1:27   ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Junio C Hamano
  2022-11-30  8:23   ` [PATCH v3 0/1] " Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-29 14:09 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

Instead of declaring that we'll generate e.g. any "%.1" from a
corresponding "%.xml" let's narrow that list down to only our known
manpage files, and likewise for %.xml.

We already generated e.g. "man1" on the basis of "$(DOC_MAN1)", we
just weren't keeping track of what we were generating exactly in the
these middle steps.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 89929e3d60b..f84b54ac093 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -356,14 +356,14 @@ manpage-base-url.xsl: manpage-base-url.xsl.in
 	$(QUIET_GEN)sed "s|@@MAN_BASE_URL@@|$(MAN_BASE_URL)|" $< > $@
 
 $(DOC_MANN): manpage-base-url.xsl $(wildcard manpage*.xsl)
-%.1 : %.xml
-	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
-%.5 : %.xml
-	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
-%.7 : %.xml
-	$(QUIET_XMLTO)$(XMLTO) -m $(MANPAGE_XSL) $(XMLTO_EXTRA) man $<
-
-%.xml : %.txt $(ASCIIDOC_DEPS)
+define doc-mann-rule
+$$(DOC_MAN$(1)) : %.$(1) : %.xml
+	$$(QUIET_XMLTO)$$(XMLTO) -m $$(MANPAGE_XSL) $$(XMLTO_EXTRA) man $$<
+
+endef
+$(eval $(foreach n,1 5 7,$(call doc-mann-rule,$(n))))
+
+$(MAN_XML): %.xml : %.txt $(ASCIIDOC_DEPS)
 	$(QUIET_ASCIIDOC)$(TXT_TO_XML) -d manpage -o $@ $<
 
 user-manual.xml: user-manual.txt user-manual.conf asciidoctor-extensions.rb GIT-ASCIIDOCFLAGS
-- 
2.39.0.rc0.993.g0c499e58e3b


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2022-11-29 14:09   ` [PATCH v2 4/4] Documentation/Makefile: narrow wildcard rules to our known files Ævar Arnfjörð Bjarmason
@ 2022-11-30  1:27   ` Junio C Hamano
  2022-11-30  8:23   ` [PATCH v3 0/1] " Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2022-11-30  1:27 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Paul Smith

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

> GNU Make 4.4 was released just about a month ago[1], this series picks
> up & amends a change from Paul Smith (the GNU make maintainer), and
> then fixes another bug in our Makefiles as a result of a
> backwards-incompatible change in how $(MAKEFLAGS) works in 4.4.
>
> Junio: I think this is worth considering for merging down in the rc
> peried.

"in the rc period" -> "before -rc1".

Yes I was planning to merge down Paul's topic which was very much
minimum and obvious.  I do not think "while at it, make it less

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 0/1] Makefiles: GNU make 4.4 fixes
  2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2022-11-30  1:27   ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Junio C Hamano
@ 2022-11-30  8:23   ` Ævar Arnfjörð Bjarmason
  2022-11-30  8:23     ` [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-30  8:23 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

A now much-smaller re-roll of a potential for-v2.39.0 fix for GNU make
4.4 compatibility.

Junio: Sorry about the overlapping submission, at the time I didn't
see Paul's in the "What's Cooking", and thought it hadn't been picked
up at all (maybe I just forgot to look at the actual branches).

This v3 is just the "MAKEFLAGS" patch. I agree with your [2] that we
might want to leave this post-release, i.e. it'll just be (a lot) more
verbose, but does it break anything? Probably not.

On the other hand the fix here is trivial, and literally just the
exact solution to this compatibility problem suggested by GNU make's
"NEWS" file, and nothing else. So merging this before the release
should be low-risk...

1. https://lore.kernel.org/git/cover-v2-0.4-00000000000-20221129T140159Z-avarab@gmail.com/
2. https://lore.kernel.org/git/xmqqk03dyskc.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (1):
  Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4

 git-gui/Makefile | 2 +-
 shared.mak       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Range-diff against v2:
1:  42b4f241c97 < -:  ----------- Documentation/Makefile: de-duplicate *.[157] dependency list
2:  e232f308e40 < -:  ----------- Documentation/Makefile: avoid multiple patterns when generating one file
3:  6db7dd74e52 = 1:  432518b2dd7 Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
4:  f1bc3c16904 < -:  ----------- Documentation/Makefile: narrow wildcard rules to our known files
-- 
2.39.0.rc0.1028.gb88f24da998

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30  8:23   ` [PATCH v3 0/1] " Ævar Arnfjörð Bjarmason
@ 2022-11-30  8:23     ` Ævar Arnfjörð Bjarmason
  2022-11-30 16:29       ` Paul Smith
  0 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-30  8:23 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Paul Smith,
	Ævar Arnfjörð Bjarmason

Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
changed in a backward-incompatible way, as its "NEWS" file notes:

  Previously only simple (one-letter) options were added to the MAKEFLAGS
  variable that was visible while parsing makefiles.  Now, all options are
  available in MAKEFLAGS.  If you want to check MAKEFLAGS for a one-letter
  option, expanding "$(firstword -$(MAKEFLAGS))" is a reliable way to return
  the set of one-letter options which can be examined via findstring, etc.

This upstream change meant that e.g.:

	make man

Would become very noisy, because in shared.mak we rely on extracting
"s" from the $(MAKEFLAGS), which now contains long options like
"--jobserver-auth=fifo:<path>", which we'll conflate with the "-s"
option.

So, let's change this idiom we've been carrying since [1], [2] and [3]
as the "NEWS" suggests.

Note that the "-" in "-$(MAKEFLAGS)" is critical here, as the variable
will always contain leading whitespace if there are no short options,
but long options are present. Without it e.g. "make --debug=all" would
yield "--debug=all" as the first word, but with it we'll get "-" as
intended. Then "-s" for "-s", "-Bs" for "-s -B" etc.

1. 0c3b4aac8ec (git-gui: Support of "make -s" in: do not output
   anything of the build itself, 2007-03-07)
2. b777434383b (Support of "make -s": do not output anything of the
   build itself, 2007-03-07)
3. bb2300976ba (Documentation/Makefile: make most operations "quiet",
   2009-03-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-gui/Makefile | 2 +-
 shared.mak       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-gui/Makefile b/git-gui/Makefile
index 56c85a85c1e..a0d5a4b28e1 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile
@@ -116,7 +116,7 @@ ifeq ($(uname_S),Darwin)
 	TKEXECUTABLE = $(shell basename "$(TKFRAMEWORK)" .app)
 endif

-ifeq ($(findstring $(MAKEFLAGS),s),s)
+ifeq ($(findstring $(firstword -$(MAKEFLAGS)),s),s)
 QUIET_GEN =
 endif

diff --git a/shared.mak b/shared.mak
index be1f30ff206..aeb80fc4d5a 100644
--- a/shared.mak
+++ b/shared.mak
@@ -37,13 +37,13 @@ space := $(empty) $(empty)
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =

-ifneq ($(findstring w,$(MAKEFLAGS)),w)
+ifneq ($(findstring w,$(firstword -$(MAKEFLAGS))),w)
 PRINT_DIR = --no-print-directory
 else # "make -w"
 NO_SUBDIR = :
 endif

-ifneq ($(findstring s,$(MAKEFLAGS)),s)
+ifneq ($(findstring s,$(firstword -$(MAKEFLAGS))),s)
 ifndef V
 ## common
 	QUIET_SUBDIR0  = +@subdir=
-- 
2.39.0.rc0.1028.gb88f24da998

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30  8:23     ` [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
@ 2022-11-30 16:29       ` Paul Smith
  2022-11-30 22:23         ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Paul Smith @ 2022-11-30 16:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano

On Wed, 2022-11-30 at 09:23 +0100, Ævar Arnfjörð Bjarmason wrote:
> Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
> changed in a backward-incompatible way, as its "NEWS" file notes:

Hrm.  I did try to look through the other makefiles to find similar
constructs and get them all, but apparently my grep fu was
insufficient.  Bother.

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30 16:29       ` Paul Smith
@ 2022-11-30 22:23         ` Junio C Hamano
  2022-12-06  7:48           ` Johannes Schindelin
  0 siblings, 1 reply; 23+ messages in thread
From: Junio C Hamano @ 2022-11-30 22:23 UTC (permalink / raw)
  To: Paul Smith; +Cc: Ævar Arnfjörð Bjarmason, git

Paul Smith <psmith@gnu.org> writes:

> On Wed, 2022-11-30 at 09:23 +0100, Ævar Arnfjörð Bjarmason wrote:
>> Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
>> changed in a backward-incompatible way, as its "NEWS" file notes:
>
> Hrm.  I did try to look through the other makefiles to find similar
> constructs and get them all, but apparently my grep fu was
> insufficient.  Bother.
>
> Thanks.

Thanks, both.  Will queue.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-11-30 22:23         ` Junio C Hamano
@ 2022-12-06  7:48           ` Johannes Schindelin
  2022-12-06  8:13             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 23+ messages in thread
From: Johannes Schindelin @ 2022-12-06  7:48 UTC (permalink / raw)
  To: Junio C Hamano, Pratyush Yadav
  Cc: Paul Smith, Ævar Arnfjörð Bjarmason, git

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Hi Junio,

On Thu, 1 Dec 2022, Junio C Hamano wrote:

> Paul Smith <psmith@gnu.org> writes:
>
> > On Wed, 2022-11-30 at 09:23 +0100, Ævar Arnfjörð Bjarmason wrote:
> >> Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
> >> changed in a backward-incompatible way, as its "NEWS" file notes:
> >
> > Hrm.  I did try to look through the other makefiles to find similar
> > constructs and get them all, but apparently my grep fu was
> > insufficient.  Bother.
> >
> > Thanks.
>
> Thanks, both.  Will queue.

I noticed that this patch also touches Git GUI, a change which technically
should have come in via https://github.com/prati0100/git-gui, not directly
via git/git.

So let's make Pratyush [Cc:ed] aware of this change.

We probably want to avoid applying Git GUI changes directly to git/git in
the future. In the meantime, because I know that Pratyush is busy, I
opened https://github.com/prati0100/git-gui/pull/83 with a (partial)
backport of this patch.

The following command demonstrates that this change is the only divergence
that would need backporting into Git GUI (the first SHA is the current tip
of git/git's `main` and the second SHA is the latest git-gui tip that was
merged into git/git):

	git diff 2e71cbbddd6:git-gui df4f9e28f64:

For the record, there is one change in git-gui's main branch that has not
yet made it into git/git [*1*], but it merely appends a full stop
character to the end of a sentence in the `README.md` file, therefore
there is probably no urgency in pulling git-gui into git any time soon.
That typo fix waited over a year to make it into git/git, it can easily
wait some more.

Ciao,
Johannes

Footnote *1*: https://github.com/prati0100/git-gui/commit/8cf36407cab

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-12-06  7:48           ` Johannes Schindelin
@ 2022-12-06  8:13             ` Ævar Arnfjörð Bjarmason
  2022-12-06  9:13               ` Junio C Hamano
  0 siblings, 1 reply; 23+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-12-06  8:13 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, Pratyush Yadav, Paul Smith, git


On Tue, Dec 06 2022, Johannes Schindelin wrote:

> Hi Junio,
>
> On Thu, 1 Dec 2022, Junio C Hamano wrote:
>
>> Paul Smith <psmith@gnu.org> writes:
>>
>> > On Wed, 2022-11-30 at 09:23 +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> Since GNU make 4.4 the semantics of the $(MAKEFLAGS) variable has
>> >> changed in a backward-incompatible way, as its "NEWS" file notes:
>> >
>> > Hrm.  I did try to look through the other makefiles to find similar
>> > constructs and get them all, but apparently my grep fu was
>> > insufficient.  Bother.
>> >
>> > Thanks.
>>
>> Thanks, both.  Will queue.
>
> I noticed that this patch also touches Git GUI, a change which technically
> should have come in via https://github.com/prati0100/git-gui, not directly
> via git/git.
> 
> I noticed that this patch also touches Git GUI, a change which technically
> should have come in via https://github.com/prati0100/git-gui, not directly
> via git/git.
> 
> So let's make Pratyush [Cc:ed] aware of this change.
> 
> We probably want to avoid applying Git GUI changes directly to git/git in
> the future. In the meantime, because I know that Pratyush is busy, I
> opened https://github.com/prati0100/git-gui/pull/83 with a (partial)
> backport of this patch.

Should it? I looked at https://github.com/prati0100/git-gui#contributing
before including git-gui in that change, which says:

	Even though the project is hosted at GitHub, the development
	does not happen over GitHub Issues and Pull Requests.  Instead,
	an email based workflow is used. The Git mailing list
	[git@vger.kernel.org](mailto:git@vger.kernel.org) is where the
	patches are discussed and reviewed.

As a bit of deja-vu when trying to find if that was outdated or not I
found that you seemed to have had pretty much this exact exchange
already with the git-gui maintainer at
https://lore.kernel.org/git/20190924122306.bcwe37wlahjimve7@yadavpratyush.com/

Which seems to have been followed-up by
https://lore.kernel.org/git/pull.361.git.gitgitgadget@gmail.com/;
I.e. you sent a git-gui change to this ML.

Or do you mean that it should have been sent to this ML, Pratyush should
have pulled it, and Junio would have pulled upstream after that?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4
  2022-12-06  8:13             ` Ævar Arnfjörð Bjarmason
@ 2022-12-06  9:13               ` Junio C Hamano
  0 siblings, 0 replies; 23+ messages in thread
From: Junio C Hamano @ 2022-12-06  9:13 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Johannes Schindelin, Pratyush Yadav, Paul Smith, git

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

> Which seems to have been followed-up by
> https://lore.kernel.org/git/pull.361.git.gitgitgadget@gmail.com/;
> I.e. you sent a git-gui change to this ML.
>
> Or do you mean that it should have been sent to this ML, Pratyush should
> have pulled it, and Junio would have pulled upstream after that?

The destination of the e-mailed patch was fine.  I think what Dscho
is saying is that the patch for git-gui should have been split into
its own patch that is rooted at that project, i.e. the "diff --git"
line shouldn't have had "a/git-gui/Makefile" but just "a/Makefile"
if the patch were to modify the top-level Makefile of that project.

Then the git-gui maintainer picks up the patch (after possible
review iterations), applies to his or her tree, and tells me to pull
the result with "-Xsubtree=git-gui" option.

At least that was how the world worked, when we had an active
git-gui maintainer.  The same story goes for gitk part of the tree.

These days, neither subtree is very active and I am not sure how
much value we are getting out of this "clean separation".

Thanks.

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-12-06  9:13 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-27 22:42 [PATCH 0/1] Avoid multiple patterns when recipes generate one file Paul Smith
2022-11-27 22:42 ` [PATCH 1/1] " Paul Smith
2022-11-28 13:08   ` Ævar Arnfjörð Bjarmason
2022-11-28 18:33     ` Paul Smith
2022-11-28 18:57       ` Ævar Arnfjörð Bjarmason
2022-11-29 14:09 ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Ævar Arnfjörð Bjarmason
2022-11-29 14:09   ` [PATCH v2 1/4] Documentation/Makefile: de-duplicate *.[157] dependency list Ævar Arnfjörð Bjarmason
2022-11-30  4:17     ` Junio C Hamano
2022-11-29 14:09   ` [PATCH v2 2/4] Documentation/Makefile: avoid multiple patterns when generating one file Ævar Arnfjörð Bjarmason
2022-11-30  4:18     ` Junio C Hamano
2022-11-29 14:09   ` [PATCH v2 3/4] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
2022-11-30  4:28     ` Junio C Hamano
2022-11-30  5:49       ` Paul Smith
2022-12-01 12:37         ` Ævar Arnfjörð Bjarmason
2022-11-29 14:09   ` [PATCH v2 4/4] Documentation/Makefile: narrow wildcard rules to our known files Ævar Arnfjörð Bjarmason
2022-11-30  1:27   ` [PATCH v2 0/4] Makefiles: GNU make 4.4 fixes Junio C Hamano
2022-11-30  8:23   ` [PATCH v3 0/1] " Ævar Arnfjörð Bjarmason
2022-11-30  8:23     ` [PATCH v3 1/1] Makefiles: change search through $(MAKEFLAGS) for GNU make 4.4 Ævar Arnfjörð Bjarmason
2022-11-30 16:29       ` Paul Smith
2022-11-30 22:23         ` Junio C Hamano
2022-12-06  7:48           ` Johannes Schindelin
2022-12-06  8:13             ` Ævar Arnfjörð Bjarmason
2022-12-06  9:13               ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).