Patchwork [RESEND,for-1.4] make_device_config.sh: Fix target path in generated dependency file

login
register
mail settings
Submitter Andreas Färber
Date Jan. 24, 2013, 3:47 p.m.
Message ID <1359042475-4592-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/215374/
State New
Headers show

Comments

Andreas Färber - Jan. 24, 2013, 3:47 p.m.
config-devices.mak.d is included from Makefile.target, i.e. from inside
the *-softmmu/ directory. It included the directory path, so never
applied to the actual ./config-devices.mak. Symptoms were spurious
build failures due to missing dependency on default-configs/pci.mak.

Fix this by using `basename` to strip the directory path.

Reported-by: Gerhard Wiesinger <lists@wiesinger.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 scripts/make_device_config.sh |    2 +-
 1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
Blue Swirl - Jan. 26, 2013, 3:08 p.m.
Thanks, applied.

On Thu, Jan 24, 2013 at 3:47 PM, Andreas Färber <afaerber@suse.de> wrote:
> config-devices.mak.d is included from Makefile.target, i.e. from inside
> the *-softmmu/ directory. It included the directory path, so never
> applied to the actual ./config-devices.mak. Symptoms were spurious
> build failures due to missing dependency on default-configs/pci.mak.
>
> Fix this by using `basename` to strip the directory path.
>
> Reported-by: Gerhard Wiesinger <lists@wiesinger.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  scripts/make_device_config.sh |    2 +-
>  1 Datei geändert, 1 Zeile hinzugefügt(+), 1 Zeile entfernt(-)
>
> diff --git a/scripts/make_device_config.sh b/scripts/make_device_config.sh
> index 5d14885..0778fe2 100644
> --- a/scripts/make_device_config.sh
> +++ b/scripts/make_device_config.sh
> @@ -25,4 +25,4 @@ done
>  process_includes $src > $dest
>
>  cat $src $all_includes | grep -v '^include' > $dest
> -echo "$1: $all_includes" > $dep
> +echo "`basename $1`: $all_includes" > $dep
> --
> 1.7.10.4
>
>
Peter Maydell - Feb. 21, 2013, 12:34 p.m.
On 24 January 2013 15:47, Andreas Färber <afaerber@suse.de> wrote:
> config-devices.mak.d is included from Makefile.target, i.e. from inside
> the *-softmmu/ directory. It included the directory path, so never
> applied to the actual ./config-devices.mak. Symptoms were spurious
> build failures due to missing dependency on default-configs/pci.mak.
>
> Fix this by using `basename` to strip the directory path.

This patch seems to break the case it claims to be fixing:

(1) touching arm-softmmu.mak correctly provokes us to rebuild:

cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.
cam-vm-266:precise:qemu$ touch default-configs/arm-softmmu.mak
cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
/bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.

(2) but touching pci.mak does not:

cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.
cam-vm-266:precise:qemu$ touch default-configs/pci.mak
cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.

(3) git revert 23bf49b5ec and rm arm-softmmu/config-devices.mak*
and then things work:
cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.
cam-vm-266:precise:qemu$ touch default-configs/arm-softmmu.mak
cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
/bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.
cam-vm-266:precise:qemu$ touch default-configs/pci.mak
cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
/bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
make: `arm-softmmu/config-devices.mak' is up to date.


In particular the commit message claims "config-devices.mak.d
is included from Makefile.target" but this is wrong -- this
is done from the top level Makefile via the line
"-include $(SUBDIR_DEVICES_MAK_DEP)". This is why you need
to have the full path in the emitted dependency line.

-- PMM
Andreas Färber - Feb. 21, 2013, 12:52 p.m.
Am 21.02.2013 13:34, schrieb Peter Maydell:
> On 24 January 2013 15:47, Andreas Färber <afaerber@suse.de> wrote:
>> config-devices.mak.d is included from Makefile.target, i.e. from inside
>> the *-softmmu/ directory. It included the directory path, so never
>> applied to the actual ./config-devices.mak. Symptoms were spurious
>> build failures due to missing dependency on default-configs/pci.mak.
>>
>> Fix this by using `basename` to strip the directory path.
> 
> This patch seems to break the case it claims to be fixing:
> 
> (1) touching arm-softmmu.mak correctly provokes us to rebuild:
> 
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> cam-vm-266:precise:qemu$ touch default-configs/arm-softmmu.mak
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> /bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
> arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
> cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> 
> (2) but touching pci.mak does not:
> 
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> cam-vm-266:precise:qemu$ touch default-configs/pci.mak
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> 
> (3) git revert 23bf49b5ec and rm arm-softmmu/config-devices.mak*
> and then things work:
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> cam-vm-266:precise:qemu$ touch default-configs/arm-softmmu.mak
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> /bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
> arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
> cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> cam-vm-266:precise:qemu$ touch default-configs/pci.mak
> cam-vm-266:precise:qemu$ make V=1 arm-softmmu/config-devices.mak
> /bin/sh /home/petmay01/linaro/qemu-from-laptop/qemu/scripts/make_device_config.sh
> arm-softmmu/config-devices.mak default-configs/arm-softmmu.mak
> cat  arm-softmmu/config-devices.mak | grep =y | sort -u > config-all-devices.mak
> make: `arm-softmmu/config-devices.mak' is up to date.
> 
> 
> In particular the commit message claims "config-devices.mak.d
> is included from Makefile.target" but this is wrong -- this
> is done from the top level Makefile via the line
> "-include $(SUBDIR_DEVICES_MAK_DEP)". This is why you need
> to have the full path in the emitted dependency line.

As discussed on IRC, what happened here it seems is that at the time of
writing the patch (two releases ago) we had -include *.d in
Makefile.target, so the generated .mak.d files were effectively included
from two Makefiles, working for one but not for the other. So
effectively we were observing race conditions between Makefile and
x86_64-softmmu/Makefile a.k.a. Makefile.target.
This seems to have gotten fixed through Paolo's nested-vars handling in
rules.mak, which only does -include *.d for directories listed in obj-y
etc. So my patch was broken and is now completely broken. :(

However I don't see in the curent Makefile.target and rules.mak where
the *.d files are being included today, so we may have discovered a
different issue that if fixed may reintroduce the original bug again...

Paolo?

Andreas
Paolo Bonzini - Feb. 21, 2013, 1:19 p.m.
Il 21/02/2013 13:52, Andreas Färber ha scritto:
> As discussed on IRC, what happened here it seems is that at the time of
> writing the patch (two releases ago) we had -include *.d in
> Makefile.target, so the generated .mak.d files were effectively included
> from two Makefiles, working for one but not for the other. So
> effectively we were observing race conditions between Makefile and
> x86_64-softmmu/Makefile a.k.a. Makefile.target.
> This seems to have gotten fixed through Paolo's nested-vars handling in
> rules.mak, which only does -include *.d for directories listed in obj-y
> etc. So my patch was broken and is now completely broken. :(
> 
> However I don't see in the curent Makefile.target and rules.mak where
> the *.d files are being included today

Makefile.target also includes Makefile.objs and calls the unnesting
machinery in rules.mak.  The machinery then includes the .d files.

Paolo

> , so we may have discovered a
> different issue that if fixed may reintroduce the original bug again...
Andreas Färber - Feb. 21, 2013, 2:28 p.m.
Am 21.02.2013 14:19, schrieb Paolo Bonzini:
> Il 21/02/2013 13:52, Andreas Färber ha scritto:
>> As discussed on IRC, what happened here it seems is that at the time of
>> writing the patch (two releases ago) we had -include *.d in
>> Makefile.target, so the generated .mak.d files were effectively included
>> from two Makefiles, working for one but not for the other. So
>> effectively we were observing race conditions between Makefile and
>> x86_64-softmmu/Makefile a.k.a. Makefile.target.
>> This seems to have gotten fixed through Paolo's nested-vars handling in
>> rules.mak, which only does -include *.d for directories listed in obj-y
>> etc. So my patch was broken and is now completely broken. :(
>>
>> However I don't see in the curent Makefile.target and rules.mak where
>> the *.d files are being included today
> 
> Makefile.target also includes Makefile.objs and calls the unnesting
> machinery in rules.mak.  The machinery then includes the .d files.

Sure, I'm aware of the unnesting. What I meant is that unnest-vars in
rules.mak seems to -include only subdirectories added to the
to-be-unnested variables $(var) iiuc. Where does ./*.d get -include'd
though? There's no obj-y += ./ to trigger that.

Andreas

P.S. I'm preparing a revert of my patch and intend to change the file
placement to fix the conflict and avoid more misunderstandings.

> 
> Paolo
> 
>> , so we may have discovered a
>> different issue that if fixed may reintroduce the original bug again...
>
Paolo Bonzini - Feb. 21, 2013, 2:40 p.m.
Il 21/02/2013 15:28, Andreas Färber ha scritto:
>>> However I don't see in the curent Makefile.target and rules.mak where
>>> the *.d files are being included today
>>
>> Makefile.target also includes Makefile.objs and calls the unnesting
>> machinery in rules.mak.  The machinery then includes the .d files.
> 
> Sure, I'm aware of the unnesting. What I meant is that unnest-vars in
> rules.mak seems to -include only subdirectories added to the
> to-be-unnested variables $(var) iiuc. Where does ./*.d get -include'd
> though? There's no obj-y += ./ to trigger that.

Ah, I see what you mean now.

$(dir foo.o) is ./ and that causes rules.mak to include ./*.d.

Paolo

Patch

diff --git a/scripts/make_device_config.sh b/scripts/make_device_config.sh
index 5d14885..0778fe2 100644
--- a/scripts/make_device_config.sh
+++ b/scripts/make_device_config.sh
@@ -25,4 +25,4 @@  done
 process_includes $src > $dest
 
 cat $src $all_includes | grep -v '^include' > $dest
-echo "$1: $all_includes" > $dep
+echo "`basename $1`: $all_includes" > $dep