[Qemu-devel] [PATCH v2 0/2] make: Cleanup and fix of loading of dependency info

[Qemu-devel] [PATCH v2 0/2] make: Cleanup and fix of loading of dependency info

[Victor Kaplansky]

Hi,

sometimes the make fails when one switches between commits without running
"make clean".

This is caused by loading old *.d dependency info files and is harmful for
autogenerated sources with their own includes.  This situation may
significantly slow down the process of git bisect.

These two patches clean things up and fix the issue both for further versions,
and between old and new commits.

This also replaces my previous patch "[PATCH] make: explicit dependencies for
ACPI gen sources".

I've tested the fix by validating that lists included by previous "*.d"
approach and new "patsubst" approach are identical.

Victor Kaplansky (2):
  make: fix where dependency *.d are stored.
  make: load only required dependency files.

 hw/i386/Makefile.objs     | 2 ++
 hw/i386/acpi-dsdt.dsl     | 1 -
 hw/i386/q35-acpi-dsdt.dsl | 1 +
 rules.mak                 | 4 ++--
 4 files changed, 5 insertions(+), 3 deletions(-)

-- 
--Victor

[Qemu-devel] [PATCH v2 1/2] make: fix where dependency *.d are stored.

[Victor Kaplansky]

In rules like "bar/%.o: %.c" there is a difference between $(*D) and
$(@D). It is cleaner to generate *.d next to appropriate *.o, because it
allows precise including of dependency info from .d files.

As a hack, we also touch two sources for generated *.hex files. This is
to ensure *.hex rebuild, when old revision is switched to the new one.

Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
 hw/i386/acpi-dsdt.dsl     | 1 -
 hw/i386/q35-acpi-dsdt.dsl | 1 +
 rules.mak                 | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a2d84ec..8dba096 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -43,7 +43,6 @@ DefinitionBlock (
 
 #include "acpi-dsdt-hpet.dsl"
 
-
 /****************************************************************
  * PIIX4 PM
  ****************************************************************/
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 16eaca3..7be7b37 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -22,6 +22,7 @@
  * Based on acpi-dsdt.dsl, but heavily modified for q35 chipset.
  */
 
+
 ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode
 
 DefinitionBlock (
diff --git a/rules.mak b/rules.mak
index aec27f8..6e35c36 100644
--- a/rules.mak
+++ b/rules.mak
@@ -17,7 +17,7 @@ MAKEFLAGS += -rR
 QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls, $(QEMU_CFLAGS))
 
 # Flags for dependency generation
-QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
+QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
 
 # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
 QEMU_INCLUDES += -I$(<D) -I$(@D)
-- 
--Victor

Re: [Qemu-devel] [PATCH v2 1/2] make: fix where dependency *.d are stored.

[Alex Bennée]

Victor Kaplansky <victork@redhat.com> writes:

> In rules like "bar/%.o: %.c" there is a difference between $(*D) and
> $(@D). It is cleaner to generate *.d next to appropriate *.o, because it
> allows precise including of dependency info from .d files.

It might be worth putting the example in the comment as most people are
not familiar with the esoteric symbolic of GNU Make. AIUI in your
example $(@D) resolves to "bar" instead of "bar/%"

>
> As a hack, we also touch two sources for generated *.hex files. This is
> to ensure *.hex rebuild, when old revision is switched to the new one.

While I'm sure this was useful for testing do we really want to include
such a hack in the git history?

>
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
>  hw/i386/acpi-dsdt.dsl     | 1 -
>  hw/i386/q35-acpi-dsdt.dsl | 1 +
>  rules.mak                 | 2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a2d84ec..8dba096 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -43,7 +43,6 @@ DefinitionBlock (
>  
>  #include "acpi-dsdt-hpet.dsl"
>  
> -
>  /****************************************************************
>   * PIIX4 PM
>   ****************************************************************/
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 16eaca3..7be7b37 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -22,6 +22,7 @@
>   * Based on acpi-dsdt.dsl, but heavily modified for q35 chipset.
>   */
>  
> +
>  ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode
>  
>  DefinitionBlock (
> diff --git a/rules.mak b/rules.mak
> index aec27f8..6e35c36 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -17,7 +17,7 @@ MAKEFLAGS += -rR
>  QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls, $(QEMU_CFLAGS))
>  
>  # Flags for dependency generation
> -QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
> +QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
>  
>  # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
>  QEMU_INCLUDES += -I$(<D) -I$(@D)

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v2 1/2] make: fix where dependency *.d are stored.

[Michael S. Tsirkin]

On Thu, Aug 06, 2015 at 09:57:03AM +0100, Alex Bennée wrote:
> 
> Victor Kaplansky <victork@redhat.com> writes:
> 
> > In rules like "bar/%.o: %.c" there is a difference between $(*D) and
> > $(@D). It is cleaner to generate *.d next to appropriate *.o, because it
> > allows precise including of dependency info from .d files.
> 
> It might be worth putting the example in the comment as most people are
> not familiar with the esoteric symbolic of GNU Make. AIUI in your
> example $(@D) resolves to "bar" instead of "bar/%"
> 
> >
> > As a hack, we also touch two sources for generated *.hex files. This is
> > to ensure *.hex rebuild, when old revision is switched to the new one.
> 
> While I'm sure this was useful for testing do we really want to include
> such a hack in the git history?

This is the hint for why it's needed there:
"This is to ensure *.hex rebuild, when old revision is switched to the
new one."
Maybe add this, making this more explicit:
	Without this hack, anyone doing "git pull; make" will not get *.hex rebuilt
	correctly since the dependency file would be missing.


> >
> > Signed-off-by: Victor Kaplansky <victork@redhat.com>
> > ---
> >  hw/i386/acpi-dsdt.dsl     | 1 -
> >  hw/i386/q35-acpi-dsdt.dsl | 1 +
> >  rules.mak                 | 2 +-
> >  3 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> > index a2d84ec..8dba096 100644
> > --- a/hw/i386/acpi-dsdt.dsl
> > +++ b/hw/i386/acpi-dsdt.dsl
> > @@ -43,7 +43,6 @@ DefinitionBlock (
> >  
> >  #include "acpi-dsdt-hpet.dsl"
> >  
> > -
> >  /****************************************************************
> >   * PIIX4 PM
> >   ****************************************************************/
> > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> > index 16eaca3..7be7b37 100644
> > --- a/hw/i386/q35-acpi-dsdt.dsl
> > +++ b/hw/i386/q35-acpi-dsdt.dsl
> > @@ -22,6 +22,7 @@
> >   * Based on acpi-dsdt.dsl, but heavily modified for q35 chipset.
> >   */
> >  
> > +
> >  ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode
> >  
> >  DefinitionBlock (
> > diff --git a/rules.mak b/rules.mak
> > index aec27f8..6e35c36 100644
> > --- a/rules.mak
> > +++ b/rules.mak
> > @@ -17,7 +17,7 @@ MAKEFLAGS += -rR
> >  QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls, $(QEMU_CFLAGS))
> >  
> >  # Flags for dependency generation
> > -QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
> > +QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> >  
> >  # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
> >  QEMU_INCLUDES += -I$(<D) -I$(@D)
> 
> -- 
> Alex Bennée

Re: [Qemu-devel] [PATCH v2 1/2] make: fix where dependency *.d are stored.

[Alex Bennée]

Michael S. Tsirkin <mst@redhat.com> writes:

> On Thu, Aug 06, 2015 at 09:57:03AM +0100, Alex Bennée wrote:
>> 
>> Victor Kaplansky <victork@redhat.com> writes:
>> 
<snip>
>> >
>> > As a hack, we also touch two sources for generated *.hex files. This is
>> > to ensure *.hex rebuild, when old revision is switched to the new one.
>> 
>> While I'm sure this was useful for testing do we really want to include
>> such a hack in the git history?
>
> This is the hint for why it's needed there:
> "This is to ensure *.hex rebuild, when old revision is switched to the
> new one."
> Maybe add this, making this more explicit:
> 	Without this hack, anyone doing "git pull; make" will not get *.hex rebuilt
> 	correctly since the dependency file would be missing.

Ahh that makes more sense :-)

-- 
Alex Bennée

[Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Victor Kaplansky]

The old rules.mak loads dependency .d files using include directive
with file glob pattern "*.d". This breaks the build when build tree has
remanent *.d files from another build.

This patch fixes this by
  - loading precise list of .d files made from *.o and *.mo.
  - specifying explicit list of required dependency info files for
     *.hex autogenerated sources.

Note that Makefile still includes some .d in rood directory by including
"*.d".

Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
 hw/i386/Makefile.objs | 2 ++
 rules.mak             | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index bd4f147..311d364 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -10,6 +10,8 @@ obj-y += acpi-build.o
 hw/i386/acpi-build.o: hw/i386/acpi-build.c \
 	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
 
+-include hw/i386/acpi-dsdt.d hw/i386/q35-acpi-dsdt.d
+
 iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
     ; then echo "$(2)"; else echo "$(3)"; fi ;)
 
diff --git a/rules.mak b/rules.mak
index 6e35c36..4551b9e 100644
--- a/rules.mak
+++ b/rules.mak
@@ -368,6 +368,6 @@ define unnest-vars
                 $(error $o added in $v but $o-objs is not set)))
         $(shell mkdir -p ./ $(sort $(dir $($v))))
         # Include all the .d files
-        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
+        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
         $(eval $v := $(filter-out %/,$($v))))
 endef
-- 
--Victor

Re: [Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Paolo Bonzini]

On 05/08/2015 19:29, Victor Kaplansky wrote:
> The old rules.mak loads dependency .d files using include directive
> with file glob pattern "*.d". This breaks the build when build tree has
> remanent *.d files from another build.
> 
> This patch fixes this by
>   - loading precise list of .d files made from *.o and *.mo.
>   - specifying explicit list of required dependency info files for
>      *.hex autogenerated sources.
> 
> Note that Makefile still includes some .d in rood directory by including
> "*.d".
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
>  hw/i386/Makefile.objs | 2 ++
>  rules.mak             | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index bd4f147..311d364 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -10,6 +10,8 @@ obj-y += acpi-build.o
>  hw/i386/acpi-build.o: hw/i386/acpi-build.c \
>  	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
>  
> +-include hw/i386/acpi-dsdt.d hw/i386/q35-acpi-dsdt.d
> +

Having to include the files manually is ugly though.

The failed builds are ugly too, but I'm not sure which is worse... :(
Failed builds happen rarely and there are other cases where you should
just "make distclean" your way out of a failure.  On the other hand,
more complicated makefiles have to be maintained always.

Paolo

Re: [Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Victor kaplansky]

On Wed, Aug 05, 2015 at 07:40:00PM +0200, Paolo Bonzini wrote:
> 
> 
> On 05/08/2015 19:29, Victor Kaplansky wrote:
> > diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> > index bd4f147..311d364 100644
> > --- a/hw/i386/Makefile.objs
> > +++ b/hw/i386/Makefile.objs
> > @@ -10,6 +10,8 @@ obj-y += acpi-build.o
> >  hw/i386/acpi-build.o: hw/i386/acpi-build.c \
> >  	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
> >  
> > +-include hw/i386/acpi-dsdt.d hw/i386/q35-acpi-dsdt.d
> > +
> 
> Having to include the files manually is ugly though.
> 
> The failed builds are ugly too, but I'm not sure which is worse... :(
> Failed builds happen rarely and there are other cases where you should
> just "make distclean" your way out of a failure.  On the other hand,
> more complicated makefiles have to be maintained always.

Actually there is no problem to keep the list of generated *.hex targets
as we do for *.o objects. Just current list is very short, only two
items, so I leaved them as is.

--Victor

Re: [Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/08/2015 19:29, Victor Kaplansky wrote:
>> The old rules.mak loads dependency .d files using include directive
>> with file glob pattern "*.d". This breaks the build when build tree has
>> remanent *.d files from another build.
>> 
>> This patch fixes this by
>>   - loading precise list of .d files made from *.o and *.mo.
>>   - specifying explicit list of required dependency info files for
>>      *.hex autogenerated sources.
>> 
>> Note that Makefile still includes some .d in rood directory by including
>> "*.d".
>> 
>> Signed-off-by: Victor Kaplansky <victork@redhat.com>
>> ---
>>  hw/i386/Makefile.objs | 2 ++
>>  rules.mak             | 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>> index bd4f147..311d364 100644
>> --- a/hw/i386/Makefile.objs
>> +++ b/hw/i386/Makefile.objs
>> @@ -10,6 +10,8 @@ obj-y += acpi-build.o
>>  hw/i386/acpi-build.o: hw/i386/acpi-build.c \
>>  	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
>>  
>> +-include hw/i386/acpi-dsdt.d hw/i386/q35-acpi-dsdt.d
>> +
>
> Having to include the files manually is ugly though.
>
> The failed builds are ugly too, but I'm not sure which is worse... :(
> Failed builds happen rarely

Not for me.

>                             and there are other cases where you should
> just "make distclean" your way out of a failure.

ccache saves.

>                                                   On the other hand,
> more complicated makefiles have to be maintained always.

Globbing in makefiles is rarely a good idea.

Including *.d is as questionable as linking *.o would be.

Re: [Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Michael S. Tsirkin]

On Wed, Aug 05, 2015 at 08:29:33PM +0300, Victor Kaplansky wrote:
> The old rules.mak loads dependency .d files using include directive
> with file glob pattern "*.d". This breaks the build when build tree has
> remanent *.d files from another build.
> 
> This patch fixes this by
>   - loading precise list of .d files made from *.o and *.mo.
>   - specifying explicit list of required dependency info files for
>      *.hex autogenerated sources.
> 
> Note that Makefile still includes some .d in rood directory by including
> "*.d".
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
>  hw/i386/Makefile.objs | 2 ++
>  rules.mak             | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index bd4f147..311d364 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -10,6 +10,8 @@ obj-y += acpi-build.o
>  hw/i386/acpi-build.o: hw/i386/acpi-build.c \
>  	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
>  
> +-include hw/i386/acpi-dsdt.d hw/i386/q35-acpi-dsdt.d
> +
>  iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
>      ; then echo "$(2)"; else echo "$(3)"; fi ;)
>  
> diff --git a/rules.mak b/rules.mak
> index 6e35c36..4551b9e 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -368,6 +368,6 @@ define unnest-vars
>                  $(error $o added in $v but $o-objs is not set)))
>          $(shell mkdir -p ./ $(sort $(dir $($v))))
>          # Include all the .d files
> -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
> +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))

Pls add space after comma, like it was in the original.

>          $(eval $v := $(filter-out %/,$($v))))
>  endef
> -- 
> --Victor

Re: [Qemu-devel] [PATCH v2 2/2] make: load only required dependency files.

[Michael S. Tsirkin]

On Wed, Aug 05, 2015 at 09:32:18PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 05, 2015 at 08:29:33PM +0300, Victor Kaplansky wrote:
...
> > diff --git a/rules.mak b/rules.mak
> > index 6e35c36..4551b9e 100644
> > --- a/rules.mak
> > +++ b/rules.mak
> > @@ -368,6 +368,6 @@ define unnest-vars
> >                  $(error $o added in $v but $o-objs is not set)))
> >          $(shell mkdir -p ./ $(sort $(dir $($v))))
> >          # Include all the .d files
> > -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
> > +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
> 
> Pls add space after comma, like it was in the original.

For patsubst comma is pattern delimiter, so if inserted it would be part
of the pattern.  Should I add a comment on this?

--Victor