diff mbox

[for-4.7] docs/build: Work around apparent bug with multi-target rules

Message ID 1461589234-31132-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrew Cooper April 25, 2016, 1 p.m. UTC
The `make` manual documents that a rule of the form

  target1 target2: prereq
  	recipe

is equivilent to

  target1: prereq
  	recipe
  target2: prereq
  	recipe

This is correct if only target1 or target2 is wanted, but is not the case if
both target1 and target2 are wanted to be rebuilt in the same pass.  In such a
case, executing the recipe to generate target1 causes the entire rule to be
considered complete, short circuiting the search to regenerate target2.

This bug can be demonstrated with the generation of documentation from pandoc
source.

  ./xen.git$ touch docs/features/templace.pandoc
  ./xen.git$ make -C docs/
  # Regenerates html/features/template.html
  ./xen.git$ make -C docs/
  # Regenerates txt/features/template.txt
  ./xen.git$ make -C docs/
  # Regenerates pdf/features/template.pdf

To work around this, there need to be three distinct rules, so the execution
of one recipe doesn't short ciruit the others.  To avoid copy&paste
duplication, introduce a metarule, and evalute it for each document target.

As $(PANDOC) is used to generate documentation from different source types,
the metarule can be extended to also encompas the rule to create pdfs from
markdown.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/Makefile | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Ian Jackson April 25, 2016, 1:35 p.m. UTC | #1
Andrew Cooper writes ("[PATCH for-4.7] docs/build: Work around apparent bug with multi-target rules"):
> The `make` manual documents that a rule of the form
> 
>   target1 target2: prereq
>   	recipe
> 
> is equivilent to
> 
>   target1: prereq
>   	recipe
>   target2: prereq
>   	recipe
> 
> This is correct if only target1 or target2 is wanted, but is not the
> case if both target1 and target2 are wanted to be rebuilt in the
> same pass.  In such a case, executing the recipe to generate target1
> causes the entire rule to be considered complete, short circuiting
> the search to regenerate target2.

This is not a bug in make.  From the manual I have here (wheezy):

     Suppose you would like to vary the prerequisites according to the
   target, much as the variable `$@' allows you to vary the commands.
   You cannot do this with multiple targets in an ordinary rule, but
   you can do it with a "static pattern rule".  *Note Static Pattern
   Rules: Static Pattern.

and (from `Pattern Intro'):

     Pattern rules may have more than one target.  Unlike normal
   rules, this does not act as many different rules with the same
   prerequisites and commands.  If a pattern rule has multiple
   targets, `make' knows that the rule's commands are responsible for
   making all of the targets.  The commands are executed only once to
   make all the targets.

So this is a bug in the Makefile.  Your patch looks like a right
approach to me.  A static pattern rule would be the other option.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.
Andrew Cooper April 25, 2016, 1:49 p.m. UTC | #2
On 25/04/16 14:50, Wei Liu wrote:
> On Mon, Apr 25, 2016 at 02:35:56PM +0100, Ian Jackson wrote:
>> Andrew Cooper writes ("[PATCH for-4.7] docs/build: Work around apparent bug with multi-target rules"):
>>> The `make` manual documents that a rule of the form
>>>
>>>   target1 target2: prereq
>>>   	recipe
>>>
>>> is equivilent to
>>>
>>>   target1: prereq
>>>   	recipe
>>>   target2: prereq
>>>   	recipe
>>>
>>> This is correct if only target1 or target2 is wanted, but is not the
>>> case if both target1 and target2 are wanted to be rebuilt in the
>>> same pass.  In such a case, executing the recipe to generate target1
>>> causes the entire rule to be considered complete, short circuiting
>>> the search to regenerate target2.
>> This is not a bug in make.  From the manual I have here (wheezy):
>>
>>      Suppose you would like to vary the prerequisites according to the
>>    target, much as the variable `$@' allows you to vary the commands.
>>    You cannot do this with multiple targets in an ordinary rule, but
>>    you can do it with a "static pattern rule".  *Note Static Pattern
>>    Rules: Static Pattern.
>>
>> and (from `Pattern Intro'):
>>
>>      Pattern rules may have more than one target.  Unlike normal
>>    rules, this does not act as many different rules with the same
>>    prerequisites and commands.  If a pattern rule has multiple
>>    targets, `make' knows that the rule's commands are responsible for
>>    making all of the targets.  The commands are executed only once to
>>    make all the targets.
>>
>> So this is a bug in the Makefile.  Your patch looks like a right
>> approach to me.  A static pattern rule would be the other option.
>>
>> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>>
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
>
> Thank you both for handling this.
>
> Andrew, I think you need to resubmit with updated subject line and
> commit message.

I will adjust and roll it into a short series with the cpuid feature
doc.  (Guess what I was doing to find this issue...)

~Andrew
Wei Liu April 25, 2016, 1:50 p.m. UTC | #3
On Mon, Apr 25, 2016 at 02:35:56PM +0100, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.7] docs/build: Work around apparent bug with multi-target rules"):
> > The `make` manual documents that a rule of the form
> > 
> >   target1 target2: prereq
> >   	recipe
> > 
> > is equivilent to
> > 
> >   target1: prereq
> >   	recipe
> >   target2: prereq
> >   	recipe
> > 
> > This is correct if only target1 or target2 is wanted, but is not the
> > case if both target1 and target2 are wanted to be rebuilt in the
> > same pass.  In such a case, executing the recipe to generate target1
> > causes the entire rule to be considered complete, short circuiting
> > the search to regenerate target2.
> 
> This is not a bug in make.  From the manual I have here (wheezy):
> 
>      Suppose you would like to vary the prerequisites according to the
>    target, much as the variable `$@' allows you to vary the commands.
>    You cannot do this with multiple targets in an ordinary rule, but
>    you can do it with a "static pattern rule".  *Note Static Pattern
>    Rules: Static Pattern.
> 
> and (from `Pattern Intro'):
> 
>      Pattern rules may have more than one target.  Unlike normal
>    rules, this does not act as many different rules with the same
>    prerequisites and commands.  If a pattern rule has multiple
>    targets, `make' knows that the rule's commands are responsible for
>    making all of the targets.  The commands are executed only once to
>    make all the targets.
> 
> So this is a bug in the Makefile.  Your patch looks like a right
> approach to me.  A static pattern rule would be the other option.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Thank you both for handling this.

Andrew, I think you need to resubmit with updated subject line and
commit message.

Wei.

> Ian.
Andrew Cooper April 25, 2016, 3:01 p.m. UTC | #4
On 25/04/16 14:35, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.7] docs/build: Work around apparent bug with multi-target rules"):
>> The `make` manual documents that a rule of the form
>>
>>   target1 target2: prereq
>>   	recipe
>>
>> is equivilent to
>>
>>   target1: prereq
>>   	recipe
>>   target2: prereq
>>   	recipe
>>
>> This is correct if only target1 or target2 is wanted, but is not the
>> case if both target1 and target2 are wanted to be rebuilt in the
>> same pass.  In such a case, executing the recipe to generate target1
>> causes the entire rule to be considered complete, short circuiting
>> the search to regenerate target2.
> This is not a bug in make.  From the manual I have here (wheezy):
>
>      Suppose you would like to vary the prerequisites according to the
>    target, much as the variable `$@' allows you to vary the commands.
>    You cannot do this with multiple targets in an ordinary rule, but
>    you can do it with a "static pattern rule".  *Note Static Pattern
>    Rules: Static Pattern.

But we don't want to vary the prerequisite with the target.  We want the
single (Patterned) prerequisite to cause a regeneration of each three of
the (Patterned) targets using the same recipe, as the $@ and $< are
sufficiently expressive for our needs.

i.e. I only chose to do it like that originally to avoid copy&pasting
the recipe.

>
> and (from `Pattern Intro'):
>
>      Pattern rules may have more than one target.  Unlike normal
>    rules, this does not act as many different rules with the same
>    prerequisites and commands.  If a pattern rule has multiple
>    targets, `make' knows that the rule's commands are responsible for
>    making all of the targets.  The commands are executed only once to
>    make all the targets.

This is the bit of documentation that I missed.  IMO, this is at least a
documentation bug, and that the multi-target documentation should
warning that multi-target pattern rules behave differently.

`make' knows, does it...  How can that possibly be the sensible choice? 
There is no automatic variable which encompasses multiple targets, and
using $* still requires you to hand-code the non-stem parts of the
targets.  This smells like a bug which was documented around.

>
> So this is a bug in the Makefile.  Your patch looks like a right
> approach to me.  A static pattern rule would be the other option.

However, a static pattern rule wouldn't avoid the repetition of the recipe.

> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks.  I will see about adjusting the commit message, but the patch
itself doesn't need to change.

~Andrew
diff mbox

Patch

diff --git a/docs/Makefile b/docs/Makefile
index b9da605..e2537e8 100644
--- a/docs/Makefile
+++ b/docs/Makefile
@@ -180,22 +180,24 @@  txt/%.txt: %.markdown
 	@$(INSTALL_DIR) $(@D)
 	$(INSTALL_DATA) $< $@
 
-pdf/%.pdf: %.markdown
-ifneq ($(PANDOC),)
-	@$(INSTALL_DIR) $(@D)
-	$(PANDOC) --number-sections --toc --standalone $< --output $@
-else
-	@echo "pandoc not installed; skipping $@"
-endif
+# Metarule for generating pandoc rules.
+define GENERATE_PANDOC_RULE
+# $(1) is the target documentation format. $(2) is the source format.
 
-pdf/%.pdf txt/%.txt html/%.html: %.pandoc
+$(1)/%.$(1): %.$(2)
 ifneq ($(PANDOC),)
-	@$(INSTALL_DIR) $(@D)
-	$(PANDOC) --number-sections --toc --standalone $< --output $@
+	@$(INSTALL_DIR) $$(@D)
+	$(PANDOC) --number-sections --toc --standalone $$< --output $$@
 else
-	@echo "pandoc not installed; skipping $@"
+	@echo "pandoc not installed; skipping $$@"
 endif
 
+endef
+$(eval $(call GENERATE_PANDOC_RULE,pdf,pandoc))   # pdf/%.pdf: %.pandoc
+$(eval $(call GENERATE_PANDOC_RULE,txt,pandoc))   # txt/%.txt: %.pandoc
+$(eval $(call GENERATE_PANDOC_RULE,html,pandoc))  # html/%.html: %.pandoc
+$(eval $(call GENERATE_PANDOC_RULE,pdf,markdown)) # pdf/%.pdf: %.markdown
+
 ifeq (,$(findstring clean,$(MAKECMDGOALS)))
 $(XEN_ROOT)/config/Docs.mk:
 	$(error You have to run ./configure before building docs)