patchAvoid multiple patterns when recipes generate one file

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

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(-)

On Sun, Nov 27 2022, Paul Smith wrote:

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

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...):

On Mon, 2022-11-28 at 14:08 +0100, Ævar Arnfjörð Bjarmason wrote:

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?

On Mon, Nov 28 2022, Paul Smith wrote:

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

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

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(-)

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

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

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.

On Wed, 2022-11-30 at 13:28 +0900, Junio C Hamano wrote:

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.

On Wed, Nov 30 2022, Paul Smith wrote:

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.

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...).

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(-)

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

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?

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(-)

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

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

Thanks, both.

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(-)

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

"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

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

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(-)

On Wed, 2022-11-30 at 09:23 +0100, Ævar Arnfjörð Bjarmason wrote:

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.

Paul Smith <psmith@gnu.org> writes:

Thanks, both.  Will queue.

Hi Junio,

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

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

On Tue, Dec 06 2022, Johannes Schindelin wrote:

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?

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

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.