Patchwork [1/2] Buildsystem fix distclean error in pixman

login
register
mail settings
Submitter Wayne Xia
Date Nov. 16, 2012, 5:05 a.m.
Message ID <1353042317-13688-2-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/199486/
State New
Headers show

Comments

Wayne Xia - Nov. 16, 2012, 5:05 a.m.
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(-)
Peter Maydell - Nov. 16, 2012, 9:27 a.m.
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
Wayne Xia - Nov. 16, 2012, 10:08 a.m.
于 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.
Peter Maydell - Nov. 16, 2012, 10:16 a.m.
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
Paolo Bonzini - Nov. 16, 2012, 10:23 a.m.
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 "@"?
Peter Maydell - Nov. 16, 2012, 10:26 a.m.
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
Wayne Xia - Nov. 16, 2012, 10:27 a.m.
于 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.
Gerd Hoffmann - Nov. 16, 2012, 10:38 a.m.
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
Wayne Xia - Nov. 16, 2012, 10:51 a.m.
于 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.
Wayne Xia - Nov. 16, 2012, 10:53 a.m.
于 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.

Patch

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 \