Patchwork [1/2] configure: Replace bash code by standard shell code

login
register
mail settings
Submitter Stefan Weil
Date July 15, 2012, 6:34 p.m.
Message ID <1342377272-32511-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/171081/
State Accepted
Headers show

Comments

Stefan Weil - July 15, 2012, 6:34 p.m.
"+=" does not work with dash and other simple /bin/sh implementations.

The new code prepends the flag while the old code either did not work
(it continued after an error message which typically was not read) or
appended the flag. That difference should not matter here.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 configure |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Peter Maydell - July 17, 2012, 6:43 p.m.
On 15 July 2012 19:34, Stefan Weil <sw@weilnetz.de> wrote:
> "+=" does not work with dash and other simple /bin/sh implementations.
>
> The new code prepends the flag while the old code either did not work
> (it continued after an error message which typically was not read) or
> appended the flag. That difference should not matter here.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  configure |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/configure b/configure
> index 12351f5..0269ba0 100755
> --- a/configure
> +++ b/configure
> @@ -2822,7 +2822,7 @@ int main(int argc, char **argv)
>  }
>  EOF
>    if ! compile_prog "" "" ; then
> -    CFLAGS+="-march=i486"
> +    CFLAGS="-march=i486 $CFLAGS"
>    fi
>  fi

This is not quite the right fix for this. This flag should be in
QEMU_CFLAGS, because it is a flag without which QEMU
would be unable to compile. See previous discussion in this
thread:

http://lists.xen.org/archives/html/xen-devel/2012-04/msg00330.html

(Unfortunately Olaf never submitted an updated patch.)

-- PMM
Stefan Weil - July 17, 2012, 7:07 p.m.
Am 17.07.2012 20:43, schrieb Peter Maydell:
> On 15 July 2012 19:34, Stefan Weil <sw@weilnetz.de> wrote:
>> "+=" does not work with dash and other simple /bin/sh implementations.
>>
>> The new code prepends the flag while the old code either did not work
>> (it continued after an error message which typically was not read) or
>> appended the flag. That difference should not matter here.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>   configure |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 12351f5..0269ba0 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2822,7 +2822,7 @@ int main(int argc, char **argv)
>>   }
>>   EOF
>>     if ! compile_prog "" "" ; then
>> -    CFLAGS+="-march=i486"
>> +    CFLAGS="-march=i486 $CFLAGS"
>>     fi
>>   fi
>
> This is not quite the right fix for this. This flag should be in
> QEMU_CFLAGS, because it is a flag without which QEMU
> would be unable to compile. See previous discussion in this
> thread:
>
> http://lists.xen.org/archives/html/xen-devel/2012-04/msg00330.html
>
> (Unfortunately Olaf never submitted an updated patch.)
>
> -- PMM
>

If the user overrides CFLAGS, I expect that he/she will notice
that compilation fails and hopefully find the cause (only
expert users should override CFLAGS). Overriding -march=i486
might be useful to set -march=i686.

Therefore it was not so obvious to me whether CFLAGS or
QEMU_CFLAGS was the better choice.

I don't mind if we decide to use QEMU_CFLAGS here,
but suggest putting that change in a separate patch.

This one fixes a shell syntax error without changing the macro name.

Regards,

Stefan W.
Peter Maydell - July 17, 2012, 7:19 p.m.
On 17 July 2012 20:07, Stefan Weil <sw@weilnetz.de> wrote:
> If the user overrides CFLAGS, I expect that he/she will notice
> that compilation fails and hopefully find the cause (only
> expert users should override CFLAGS). Overriding -march=i486
> might be useful to set -march=i686.
>
> Therefore it was not so obvious to me whether CFLAGS or
> QEMU_CFLAGS was the better choice.

Yes, I had to dig to find out the underlying principle
distinguishing the two. Having found it I would like us to
fix this in a way which respects that principle.

-- PMM
Andreas Färber - July 20, 2012, 12:28 p.m.
Am 17.07.2012 21:19, schrieb Peter Maydell:
> On 17 July 2012 20:07, Stefan Weil <sw@weilnetz.de> wrote:
>> If the user overrides CFLAGS, I expect that he/she will notice
>> that compilation fails and hopefully find the cause (only
>> expert users should override CFLAGS). Overriding -march=i486
>> might be useful to set -march=i686.
>>
>> Therefore it was not so obvious to me whether CFLAGS or
>> QEMU_CFLAGS was the better choice.
> 
> Yes, I had to dig to find out the underlying principle
> distinguishing the two. Having found it I would like us to
> fix this in a way which respects that principle.

I was planning to pick up Olaf's patch doing just that but haven't got
around to it in time for v1.1. Olaf, do you have time to resubmit a
patch to upstream qemu-devel as requested by Peter and me?

Regards,
Andreas
Peter Maydell - July 20, 2012, 12:30 p.m.
On 20 July 2012 13:28, Andreas Färber <afaerber@suse.de> wrote:
> Am 17.07.2012 21:19, schrieb Peter Maydell:
>> On 17 July 2012 20:07, Stefan Weil <sw@weilnetz.de> wrote:
>>> If the user overrides CFLAGS, I expect that he/she will notice
>>> that compilation fails and hopefully find the cause (only
>>> expert users should override CFLAGS). Overriding -march=i486
>>> might be useful to set -march=i686.
>>>
>>> Therefore it was not so obvious to me whether CFLAGS or
>>> QEMU_CFLAGS was the better choice.
>>
>> Yes, I had to dig to find out the underlying principle
>> distinguishing the two. Having found it I would like us to
>> fix this in a way which respects that principle.
>
> I was planning to pick up Olaf's patch doing just that but haven't got
> around to it in time for v1.1. Olaf, do you have time to resubmit a
> patch to upstream qemu-devel as requested by Peter and me?

I actually wrote a patch sitting on top of this one by Stefan
which moves the flag to QEMU_CFLAGS;
both are in the 11-patch series dealing with various configure
Werror issues:
 http://patchwork.ozlabs.org/patch/171689/
 http://patchwork.ozlabs.org/patch/171706/

-- PMM
Olaf Hering - July 20, 2012, 12:35 p.m.
On Fri, Jul 20, Peter Maydell wrote:

> I actually wrote a patch sitting on top of this one by Stefan
> which moves the flag to QEMU_CFLAGS;
> both are in the 11-patch series dealing with various configure
> Werror issues:
>  http://patchwork.ozlabs.org/patch/171689/

The actual CFLAGS issue had to be solved differently in the xen sources.
And the patch above solves the bug in configure.

Olaf

Patch

diff --git a/configure b/configure
index 12351f5..0269ba0 100755
--- a/configure
+++ b/configure
@@ -2822,7 +2822,7 @@  int main(int argc, char **argv)
 }
 EOF
   if ! compile_prog "" "" ; then
-    CFLAGS+="-march=i486"
+    CFLAGS="-march=i486 $CFLAGS"
   fi
 fi