[Qemu-devel] [PATCH 0/2] Buildsystem fix

[Qemu-devel] [PATCH 0/2] Buildsystem fix

[Wenchao Xia]

  Following patch fix a problem and adjust build system, to make it more
modulized and easier to extend.

Wenchao Xia (2):
  Buildsystem fix distclean error in pixman
  Buildsystem clean tests directory clearly

 Makefile       |    7 +++++--
 configure      |    2 +-
 tests/Makefile |    7 +++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

[Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Wenchao Xia]

  Currently if pixman have no config.log inside, make file still
try to clean it resulting error. This patch fix it.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 Makefile |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 81c660f..f40885b 100644
--- a/Makefile
+++ b/Makefile
@@ -278,7 +278,10 @@ distclean: clean
 	for d in $(TARGET_DIRS) $(QEMULIBS); do \
 	rm -rf $$d || exit 1 ; \
         done
-	test -f pixman/config.log && make -C pixman distclean
+	@if test -f pixman/config.log; \
+	then \
+		make -C pixman distclean;\
+	fi
 
 KEYMAPS=da     en-gb  et  fr     fr-ch  is  lt  modifiers  no  pt-br  sv \
 ar      de     en-us  fi  fr-be  hr     it  lv  nl         pl  ru     th \
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Peter Maydell]

On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>   Currently if pixman have no config.log inside, make file still
> try to clean it resulting error. This patch fix it.
>
> -       test -f pixman/config.log && make -C pixman distclean
> +       @if test -f pixman/config.log; \
> +       then \
> +               make -C pixman distclean;\
> +       fi

These two bits of shellscript both do the same thing, don't they?

-- PMM

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Wenchao Xia]

于 2012-11-16 17:27, Peter Maydell 写道:
> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>    Currently if pixman have no config.log inside, make file still
>> try to clean it resulting error. This patch fix it.
>>
>> -       test -f pixman/config.log && make -C pixman distclean
>> +       @if test -f pixman/config.log; \
>> +       then \
>> +               make -C pixman distclean;\
>> +       fi
>
> These two bits of shellscript both do the same thing, don't they?
>
> -- PMM
>
   Looks the same, but on my machine
make -C pixman distclean
got executed resulting error, I guess that is what original author
wants to avoid.
   I'll conclude this patch into my libqblock patch serials to make
it easier to be reviwed.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Peter Maydell]

On 16 November 2012 10:08, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2012-11-16 17:27, Peter Maydell 写道:
>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>> -       test -f pixman/config.log && make -C pixman distclean
>>> +       @if test -f pixman/config.log; \
>>> +       then \
>>> +               make -C pixman distclean;\
>>> +       fi

>> These two bits of shellscript both do the same thing, don't they?

>   Looks the same, but on my machine
> make -C pixman distclean
> got executed resulting error, I guess that is what original author
> wants to avoid.

If your shell executes Y in "X && Y" even if X fails, then your
shell is badly badly broken. More likely, there is some other
problem, and you need to find out what it is.
I don't think this patch should be applied, I'm afraid.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Wenchao Xia]

于 2012-11-16 18:16, Peter Maydell 写道:
> On 16 November 2012 10:08, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>> 于 2012-11-16 17:27, Peter Maydell 写道:
>>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>> -       test -f pixman/config.log && make -C pixman distclean
>>>> +       @if test -f pixman/config.log; \
>>>> +       then \
>>>> +               make -C pixman distclean;\
>>>> +       fi
>
>>> These two bits of shellscript both do the same thing, don't they?
>
>>    Looks the same, but on my machine
>> make -C pixman distclean
>> got executed resulting error, I guess that is what original author
>> wants to avoid.
>
> If your shell executes Y in "X && Y" even if X fails, then your
> shell is badly badly broken. More likely, there is some other
> problem, and you need to find out what it is.
> I don't think this patch should be applied, I'm afraid.
>
> -- PMM
>
   My shell is OK, I missed the error reporting source before,
in my test I think it is first part reporting error, that
is "test -f pixman/config.log" directly report error.
   This patch would eliminate this error.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Paolo Bonzini]

Il 16/11/2012 10:27, Peter Maydell ha scritto:
> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>   Currently if pixman have no config.log inside, make file still
>> try to clean it resulting error. This patch fix it.
>>
>> -       test -f pixman/config.log && make -C pixman distclean
>> +       @if test -f pixman/config.log; \
>> +       then \
>> +               make -C pixman distclean;\
>> +       fi
> 
> These two bits of shellscript both do the same thing, don't they?

No, when "test" fails the && exits with a failure.  The "if" exits with
a success (not the most portable thing ever, but we assume a decent
shell elsewhere).

I think this patch is ok, but why the "@"?

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Peter Maydell]

On 16 November 2012 10:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 16/11/2012 10:27, Peter Maydell ha scritto:
>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>> -       test -f pixman/config.log && make -C pixman distclean
>>> +       @if test -f pixman/config.log; \
>>> +       then \
>>> +               make -C pixman distclean;\
>>> +       fi
>>
>> These two bits of shellscript both do the same thing, don't they?
>
> No, when "test" fails the && exits with a failure.  The "if" exits with
> a success (not the most portable thing ever, but we assume a decent
> shell elsewhere).

Aha. I love shell. (This detail could usefully have been
provided in the commit message...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Gerd Hoffmann]

On 11/16/12 11:26, Peter Maydell wrote:
> On 16 November 2012 10:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 16/11/2012 10:27, Peter Maydell ha scritto:
>>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>> -       test -f pixman/config.log && make -C pixman distclean
>>>> +       @if test -f pixman/config.log; \
>>>> +       then \
>>>> +               make -C pixman distclean;\
>>>> +       fi
>>>
>>> These two bits of shellscript both do the same thing, don't they?
>>
>> No, when "test" fails the && exits with a failure.  The "if" exits with
>> a success (not the most portable thing ever, but we assume a decent
>> shell elsewhere).
> 
> Aha. I love shell. (This detail could usefully have been
> provided in the commit message...)

Agree.  Can I get a version with an updated commit message for the
pixman patch queue?

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Wenchao Xia]

于 2012-11-16 18:38, Gerd Hoffmann 写道:
> On 11/16/12 11:26, Peter Maydell wrote:
>> On 16 November 2012 10:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> Il 16/11/2012 10:27, Peter Maydell ha scritto:
>>>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>> -       test -f pixman/config.log && make -C pixman distclean
>>>>> +       @if test -f pixman/config.log; \
>>>>> +       then \
>>>>> +               make -C pixman distclean;\
>>>>> +       fi
>>>>
>>>> These two bits of shellscript both do the same thing, don't they?
>>>
>>> No, when "test" fails the && exits with a failure.  The "if" exits with
>>> a success (not the most portable thing ever, but we assume a decent
>>> shell elsewhere).
>>
>> Aha. I love shell. (This detail could usefully have been
>> provided in the commit message...)
>
> Agree.  Can I get a version with an updated commit message for the
> pixman patch queue?
>
> thanks,
>    Gerd
>
   Sure,I'll resend according to more comments later.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] Buildsystem fix distclean error in pixman

[Wenchao Xia]

于 2012-11-16 18:23, Paolo Bonzini 写道:
> Il 16/11/2012 10:27, Peter Maydell ha scritto:
>> On 16 November 2012 05:05, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>    Currently if pixman have no config.log inside, make file still
>>> try to clean it resulting error. This patch fix it.
>>>
>>> -       test -f pixman/config.log && make -C pixman distclean
>>> +       @if test -f pixman/config.log; \
>>> +       then \
>>> +               make -C pixman distclean;\
>>> +       fi
>>
>> These two bits of shellscript both do the same thing, don't they?
>
> No, when "test" fails the && exits with a failure.  The "if" exits with
> a success (not the most portable thing ever, but we assume a decent
> shell elsewhere).
>
> I think this patch is ok, but why the "@"?
>
   print out seems ugly :|, maybe I should cancel @ and adjust the
format.

-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 2/2] Buildsystem clean tests directory clearly

[Wenchao Xia]

  Currently make clean only clean tests/tcg and hard to extend.
This patch added command make check-clean, which clean all
generated files used in tests. With this command root Makefile
do not care tests clean method any more, it simply calls the
command to do it, so any more clean script could be added in
tests/Makefile make it easier to extend.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 Makefile       |    2 +-
 configure      |    2 +-
 tests/Makefile |    7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f40885b..8201e80 100644
--- a/Makefile
+++ b/Makefile
@@ -251,7 +251,7 @@ clean:
 	rm -f $(foreach f,$(GENERATED_SOURCES),$(f) $(f)-timestamp)
 	rm -rf qapi-generated
 	rm -rf qga/qapi-generated
-	$(MAKE) -C tests/tcg clean
+	MAKEFILES=./tests/Makefile $(MAKE) check-clean
 	for d in $(ALL_SUBDIRS) $(QEMULIBS) libcacard; do \
 	if test -d $$d; then $(MAKE) -C $$d $@ || exit 1; fi; \
 	rm -f $$d/qemu-options.def; \
diff --git a/configure b/configure
index f847ee2..a18e267 100755
--- a/configure
+++ b/configure
@@ -4163,7 +4163,7 @@ DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas"
 DIRS="$DIRS roms/seabios roms/vgabios"
 DIRS="$DIRS qapi-generated"
 DIRS="$DIRS libcacard libcacard/libcacard libcacard/trace"
-FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
+FILES="Makefile tests/Makefile tests/tcg/Makefile qdict-test-data.txt"
 FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
 FILES="$FILES tests/tcg/lm32/Makefile libcacard/Makefile"
 FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
diff --git a/tests/Makefile b/tests/Makefile
index 9bf0765..a286622 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -94,6 +94,7 @@ check-help:
 	@echo " make check-unit           Run qobject tests"
 	@echo " make check-block          Run block tests"
 	@echo " make check-report.html    Generates an HTML test report"
+	@echo " make check-clean          Clean the tests"
 	@echo
 	@echo "Please note that HTML reports do not regenerate if the unit tests"
 	@echo "has not changed."
@@ -148,4 +149,10 @@ check-unit: $(patsubst %,check-%, $(check-unit-y))
 check-block: $(patsubst %,check-%, $(check-block-y))
 check: check-unit check-qtest
 
+check-clean:
+	$(MAKE) -C tests/tcg clean
+	rm -f $(check-unit-y)
+	rm -f $(check-qtest-i386-y) $(check-qtest-x86_64-y) $(check-qtest-sparc64-y) $(check-qtest-sparc-y)
+	rm -f tests/*.o
+
 -include $(wildcard tests/*.d)
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/2] Buildsystem clean tests directory clearly

[Paolo Bonzini]

Il 16/11/2012 06:05, Wenchao Xia ha scritto:
>   Currently make clean only clean tests/tcg and hard to extend.
> This patch added command make check-clean, which clean all
> generated files used in tests. With this command root Makefile
> do not care tests clean method any more, it simply calls the
> command to do it, so any more clean script could be added in
> tests/Makefile make it easier to extend.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  Makefile       |    2 +-
>  configure      |    2 +-
>  tests/Makefile |    7 +++++++
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f40885b..8201e80 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -251,7 +251,7 @@ clean:
>  	rm -f $(foreach f,$(GENERATED_SOURCES),$(f) $(f)-timestamp)
>  	rm -rf qapi-generated
>  	rm -rf qga/qapi-generated
> -	$(MAKE) -C tests/tcg clean
> +	MAKEFILES=./tests/Makefile $(MAKE) check-clean

What is MAKEFILES?

Paolo

>  	for d in $(ALL_SUBDIRS) $(QEMULIBS) libcacard; do \
>  	if test -d $$d; then $(MAKE) -C $$d $@ || exit 1; fi; \
>  	rm -f $$d/qemu-options.def; \
> diff --git a/configure b/configure
> index f847ee2..a18e267 100755
> --- a/configure
> +++ b/configure
> @@ -4163,7 +4163,7 @@ DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas"
>  DIRS="$DIRS roms/seabios roms/vgabios"
>  DIRS="$DIRS qapi-generated"
>  DIRS="$DIRS libcacard libcacard/libcacard libcacard/trace"
> -FILES="Makefile tests/tcg/Makefile qdict-test-data.txt"
> +FILES="Makefile tests/Makefile tests/tcg/Makefile qdict-test-data.txt"
>  FILES="$FILES tests/tcg/cris/Makefile tests/tcg/cris/.gdbinit"
>  FILES="$FILES tests/tcg/lm32/Makefile libcacard/Makefile"
>  FILES="$FILES pc-bios/optionrom/Makefile pc-bios/keymaps"
> diff --git a/tests/Makefile b/tests/Makefile
> index 9bf0765..a286622 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -94,6 +94,7 @@ check-help:
>  	@echo " make check-unit           Run qobject tests"
>  	@echo " make check-block          Run block tests"
>  	@echo " make check-report.html    Generates an HTML test report"
> +	@echo " make check-clean          Clean the tests"
>  	@echo
>  	@echo "Please note that HTML reports do not regenerate if the unit tests"
>  	@echo "has not changed."
> @@ -148,4 +149,10 @@ check-unit: $(patsubst %,check-%, $(check-unit-y))
>  check-block: $(patsubst %,check-%, $(check-block-y))
>  check: check-unit check-qtest
>  
> +check-clean:
> +	$(MAKE) -C tests/tcg clean
> +	rm -f $(check-unit-y)
> +	rm -f $(check-qtest-i386-y) $(check-qtest-x86_64-y) $(check-qtest-sparc64-y) $(check-qtest-sparc-y)
> +	rm -f tests/*.o
> +
>  -include $(wildcard tests/*.d)
> 

Re: [Qemu-devel] [PATCH 2/2] Buildsystem clean tests directory clearly

[Peter Maydell]

On 16 November 2012 10:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 16/11/2012 06:05, Wenchao Xia ha scritto:
>> -     $(MAKE) -C tests/tcg clean
>> +     MAKEFILES=./tests/Makefile $(MAKE) check-clean
>
> What is MAKEFILES?

make uses it as a list of additional makefiles to be read,
as if they were 'include'd at the start of whatever makefile
it actually reads. This is almost certainly not what is
meant here, because this $(MAKE) invocation will reread
the makefile we're currently executing, prepend tests/Makefile
(gratuitously since our Makefile explicitly includes it
anyway) and then invoke the check-clean target, which
happens to work OK because tests/Makefile expects to be
included rather than standalone.

If this roundabout approach is intentional and not accidental
it needs a comment explaining why it's needed.

-- PMM