make: automatically include dependencies in recursive subdir rules

Am 17.06.2012 17:20, schrieb Blue Swirl:
> On Sun, Jun 17, 2012 at 2:57 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> I think I understand enough of what's going on in these rules to ensure this is
>> right.  But I could certainly use a second or third opinion...
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>>  Makefile  |    4 ++--
>>  rules.mak |    1 +
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index cce45fb..593bd9b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -405,5 +405,5 @@ tar:
>>  Makefile: $(GENERATED_HEADERS)
>>
>>  # Include automatically generated dependency files
>> --include $(wildcard *.d audio/*.d slirp/*.d block/*.d net/*.d ui/*.d qapi/*.d)
>> --include $(wildcard qga/*.d hw/*.d hw/usb/*.d qom/*.d)
>> +# All subdir dependencies come automatically from our recursive subdir rules
>> +-include $(wildcard *.d)
> 
> include $(wildcard *.d */*.d */*/*.d */*/*/*.d)
> ?

No. Due to our directory structure not all dependency files apply to
Makefile.

Anyway, the problem is that any such "fixed" wildcard expression still
silently fails when another level gets added. We've seen that simply no
one notices, and the resulting errors are weird and non-telling.
Therefore it was suggested to have some automatic way of deducing the
dependency files from .o files. (See the "Makefile: Fix dependencies for
QOM" thread.)

>> diff --git a/rules.mak b/rules.mak
>> index 4bc5e52..8c5ef95 100644
>> --- a/rules.mak
>> +++ b/rules.mak
>> @@ -94,6 +94,7 @@ define unnest-dir
>>  $(foreach var,$(nested-vars),$(call push-var,$(var),$1/))
>>  $(eval obj := $(obj)/$1)
>>  $(eval include $(SRC_PATH)/$1/Makefile.objs)
>> +$(eval -include $(wildcard $(SRC_PATH)/$1/*.d))
> 
> This assumes a build in the source directory, s/SRC_PATH/BUILD_DIR/.

I've tested with just
+$(eval -include $(wildcard $1/*.d))
and did
$ touch include/qemu/object.h

From what I see, using Anthony's original patch plus the modification
above (not the other ones), {i386,x86_64}-softmmu/hw/kvm/apic.o is still
not getting rebuilt despite its included hw/apic_internal.h depending on
sysbus.h -> qdev.h -> qemu/object.h.

They're being added via obj-$(CONFIG_KVM) += kvm/filename.o, not via the
recursive directory rule, so are not caught by Anthony's rule.

Maybe fix by some explicit Makefile-specific rule such as this?

+-include $(wildcard ../kvm/*.d)

Andreas

Am 17.06.2012 18:23, schrieb Andreas Färber:
> I've tested with just
> +$(eval -include $(wildcard $1/*.d))
> and did
> $ touch include/qemu/object.h
> 
> From what I see, using Anthony's original patch plus the modification
> above (not the other ones), {i386,x86_64}-softmmu/hw/kvm/apic.o is still
> not getting rebuilt despite its included hw/apic_internal.h depending on
> sysbus.h -> qdev.h -> qemu/object.h.
> 
> They're being added via obj-$(CONFIG_KVM) += kvm/filename.o, not via the
> recursive directory rule, so are not caught by Anthony's rule.
> 
> Maybe fix by some explicit Makefile-specific rule such as this?
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index d43f1df..1e8423a 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -11,3 +11,5 @@ obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o
> kvm/i8259.o kvm/ioapic.o kvm/i8254.o
>  obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
> 
>  obj-y := $(addprefix ../,$(obj-y))
> +
> +-include $(wildcard ../kvm/*.d)

If using hw/kvm/*.d instead, it actually works. ;)

Andreas

On 06/17/2012 12:00 PM, Andreas Färber wrote:
> Am 17.06.2012 18:23, schrieb Andreas Färber:
>> I've tested with just
>> +$(eval -include $(wildcard $1/*.d))
>> and did
>> $ touch include/qemu/object.h
>>
>>  From what I see, using Anthony's original patch plus the modification
>> above (not the other ones), {i386,x86_64}-softmmu/hw/kvm/apic.o is still
>> not getting rebuilt despite its included hw/apic_internal.h depending on
>> sysbus.h ->  qdev.h ->  qemu/object.h.
>>
>> They're being added via obj-$(CONFIG_KVM) += kvm/filename.o, not via the
>> recursive directory rule, so are not caught by Anthony's rule.
>>
>> Maybe fix by some explicit Makefile-specific rule such as this?
>>
>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>> index d43f1df..1e8423a 100644
>> --- a/hw/i386/Makefile.objs
>> +++ b/hw/i386/Makefile.objs
>> @@ -11,3 +11,5 @@ obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o
>> kvm/i8259.o kvm/ioapic.o kvm/i8254.o
>>   obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>
>>   obj-y := $(addprefix ../,$(obj-y))
>> +
>> +-include $(wildcard ../kvm/*.d)
>
> If using hw/kvm/*.d instead, it actually works. ;)

We can also just add a hw/kvm/Makefile.objs and then add kvm/ to the subdir 
rules.  Seems to fix the problem you identified.

Just doing a full build for v2.  Will send out when it completes.

Regards,

Anthony Liguori

>
> Andreas
>

Am 17.06.2012 20:46, schrieb Anthony Liguori:
> On 06/17/2012 12:00 PM, Andreas Färber wrote:
>> Am 17.06.2012 18:23, schrieb Andreas Färber:
>>> I've tested with just
>>> +$(eval -include $(wildcard $1/*.d))
>>> and did
>>> $ touch include/qemu/object.h
>>>
>>>  From what I see, using Anthony's original patch plus the modification
>>> above (not the other ones), {i386,x86_64}-softmmu/hw/kvm/apic.o is still
>>> not getting rebuilt despite its included hw/apic_internal.h depending on
>>> sysbus.h ->  qdev.h ->  qemu/object.h.
>>>
>>> They're being added via obj-$(CONFIG_KVM) += kvm/filename.o, not via the
>>> recursive directory rule, so are not caught by Anthony's rule.
>>>
>>> Maybe fix by some explicit Makefile-specific rule such as this?
>>>
>>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
>>> index d43f1df..1e8423a 100644
>>> --- a/hw/i386/Makefile.objs
>>> +++ b/hw/i386/Makefile.objs
>>> @@ -11,3 +11,5 @@ obj-$(CONFIG_KVM) += kvm/clock.o kvm/apic.o
>>> kvm/i8259.o kvm/ioapic.o kvm/i8254.o
>>>   obj-$(CONFIG_SPICE) += qxl.o qxl-logger.o qxl-render.o
>>>
>>>   obj-y := $(addprefix ../,$(obj-y))
>>> +
>>> +-include $(wildcard ../kvm/*.d)
>>
>> If using hw/kvm/*.d instead, it actually works. ;)
> 
> We can also just add a hw/kvm/Makefile.objs and then add kvm/ to the
> subdir rules.  Seems to fix the problem you identified.

No, that would be wrong wrt ppc/s390/arm. But we could move the files
from hw/kvm/ to hw/i386/.

Andreas

On 06/17/2012 02:53 PM, Andreas Färber wrote:
> Am 17.06.2012 20:46, schrieb Anthony Liguori:
>> On 06/17/2012 12:00 PM, Andreas Färber wrote:
>>> If using hw/kvm/*.d instead, it actually works. ;)
>>
>> We can also just add a hw/kvm/Makefile.objs and then add kvm/ to the
>> subdir rules.  Seems to fix the problem you identified.
>
> No, that would be wrong wrt ppc/s390/arm. But we could move the files
> from hw/kvm/ to hw/i386/.

Hrm, no, I don't think that's necessary.

Look at v2 of my patch.  kvm/ is only added by target hw makefiles 
(hw/i386/Makefile.objs).  So these devices are only even attempted to be added 
for i386.

Then within kvm/Makefile.objs, everything is guarded with CONFIG_KVM.

I think a better approach would be to:

hw/Makefiles.obj:
   obj-$(CONFIG_KVM) += kvm/

hw/kvm/Makefiles.obj:
   obj-$(CONFIG_I386) += apic.o pit.o ...

That way there could be ppc-specific KVM devices.

But that's a bigger refactoring as these devices need to be built target-specific.

Regards,

Anthony Liguori

>
> Andreas
>