[Qemu-devel] [PATCH 1/5] configure to set shell type

[Qemu-devel] [PATCH 1/5] configure to set shell type

[Lee Essen]

Adds support to configure for controlling which shell to use, defaults to "sh" as before
but adds "bash" for Solaris/Illumos builds. Plus ensures that tracetool is called with a
shell.

Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk>

--

 configure |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index afe7395..860c15d 100755
--- a/configure
+++ b/configure
@@ -101,6 +101,7 @@ audio_win_int=""
 cc_i386=i386-pc-linux-gnu-gcc
 libs_qga=""
 debug_info="yes"
+shell="sh"
 
 target_list=""
 
@@ -442,6 +443,7 @@ SunOS)
   # have to select again, because `uname -m` returns i86pc
   # even on an x86_64 box.
   solariscpu=`isainfo -k`
+  shell="bash"
   if test "${solariscpu}" = "amd64" ; then
     cpu="x86_64"
   fi
@@ -1097,7 +1099,7 @@ echo "  --disable-docs           disable documentation build"
 echo "  --disable-vhost-net      disable vhost-net acceleration support"
 echo "  --enable-vhost-net       enable vhost-net acceleration support"
 echo "  --enable-trace-backend=B Set trace backend"
-echo "                           Available backends:" $("$source_path"/scripts/tracetool --list-backends)
+echo "                           Available backends:" $($shell "$source_path"/scripts/tracetool --list-backends)
 echo "  --with-trace-file=NAME   Full PATH,NAME of file to store traces"
 echo "                           Default:trace-<pid>"
 echo "  --disable-spice          disable spice"
@@ -2654,7 +2656,7 @@ fi
 ##########################################
 # check if trace backend exists
 
-sh "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
+$shell "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
 if test "$?" -ne 0 ; then
   echo
   echo "Error: invalid trace backend"
@@ -3358,6 +3360,7 @@ echo "LIBS+=$LIBS" >> $config_host_mak
 echo "LIBS_TOOLS+=$libs_tools" >> $config_host_mak
 echo "EXESUF=$EXESUF" >> $config_host_mak
 echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
+echo "SHELL=$shell" >> $config_host_mak
 
 # generate list of library paths for linker script
 

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Andreas Färber]

Am 16.03.2012 13:02, schrieb Lee Essen:
> Adds support to configure for controlling which shell to use, defaults to "sh" as before
> but adds "bash" for Solaris/Illumos builds. Plus ensures that tracetool is called with a
> shell.
> 
> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk>
> 
> --
> 
>  configure |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index afe7395..860c15d 100755
> --- a/configure
> +++ b/configure
> @@ -101,6 +101,7 @@ audio_win_int=""
>  cc_i386=i386-pc-linux-gnu-gcc
>  libs_qga=""
>  debug_info="yes"
> +shell="sh"

This looks reasonable.

>  
>  target_list=""
>  
> @@ -442,6 +443,7 @@ SunOS)
>    # have to select again, because `uname -m` returns i86pc
>    # even on an x86_64 box.
>    solariscpu=`isainfo -k`
> +  shell="bash"

Are you sure this is safe for Solaris 9+? In
https://bugs.launchpad.net/qemu/+bug/636315 we concluded that
/usr/xpg4/bin/sh were the best choice of POSIX-compliant shell provided
on Solaris 10. Blue's script fixes were never applied I believe...

>    if test "${solariscpu}" = "amd64" ; then
>      cpu="x86_64"
>    fi
> @@ -1097,7 +1099,7 @@ echo "  --disable-docs           disable documentation build"
>  echo "  --disable-vhost-net      disable vhost-net acceleration support"
>  echo "  --enable-vhost-net       enable vhost-net acceleration support"
>  echo "  --enable-trace-backend=B Set trace backend"
> -echo "                           Available backends:" $("$source_path"/scripts/tracetool --list-backends)
> +echo "                           Available backends:" $($shell "$source_path"/scripts/tracetool --list-backends)
>  echo "  --with-trace-file=NAME   Full PATH,NAME of file to store traces"
>  echo "                           Default:trace-<pid>"
>  echo "  --disable-spice          disable spice"
> @@ -2654,7 +2656,7 @@ fi
>  ##########################################
>  # check if trace backend exists
>  
> -sh "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> +$shell "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
>  if test "$?" -ne 0 ; then
>    echo
>    echo "Error: invalid trace backend"
> @@ -3358,6 +3360,7 @@ echo "LIBS+=$LIBS" >> $config_host_mak
>  echo "LIBS_TOOLS+=$libs_tools" >> $config_host_mak
>  echo "EXESUF=$EXESUF" >> $config_host_mak
>  echo "LIBS_QGA+=$libs_qga" >> $config_host_mak
> +echo "SHELL=$shell" >> $config_host_mak

Why?

>  
>  # generate list of library paths for linker script
>  

Andreas

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Peter Maydell]

On 16 March 2012 12:02, Lee Essen <lee.essen@nowonline.co.uk> wrote:
> Adds support to configure for controlling which shell to use, defaults to "sh" as before
> but adds "bash" for Solaris/Illumos builds. Plus ensures that tracetool is called with a
> shell.

Ugh. If we have bashisms in our shell scripts/configure/makefiles etc we should
fix them, not paper over them.

If Solaris' /bin/sh isn't a POSIX sh that's a bug in Solaris :-)

> -echo "                           Available backends:" $("$source_path"/scripts/tracetool --list-backends)
> +echo "                           Available backends:" $($shell "$source_path"/scripts/tracetool --list-backends)

This shouldn't be necessary -- tracetool has a #!/bin/sh at the top.
If it needs bash then that should be fixed.

> -sh "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> +$shell "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null

...and we shouldn't need to use either 'sh' or '$shell' here...

-- PMM

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Lee Essen]

On 16 Mar 2012, at 12:14, Andreas Färber wrote:

> Am 16.03.2012 13:02, schrieb Lee Essen:
>> 
>> target_list=""
>> 
>> @@ -442,6 +443,7 @@ SunOS)
>>   # have to select again, because `uname -m` returns i86pc
>>   # even on an x86_64 box.
>>   solariscpu=`isainfo -k`
>> +  shell="bash"
> 
> Are you sure this is safe for Solaris 9+? In
> https://bugs.launchpad.net/qemu/+bug/636315 we concluded that
> /usr/xpg4/bin/sh were the best choice of POSIX-compliant shell provided
> on Solaris 10. Blue's script fixes were never applied I believe…
> 

# /usr/xpg4/bin/sh scripts/tracetool 
scripts/tracetool: line 113: syntax error at line 126: `)' unexpected

tracetool seems to require bash … I'm happy to look at resolving that, perhaps thats a better fix.

>> 
>> +echo "SHELL=$shell" >> $config_host_mak
> 
> Why?
> 

See patch 2/7 … there are several calls to tracetool that hardcode "sh" in the Makefile … again if
the better solution is removing the bash requirement then this problem goes away completely.

Lee.

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Lee Essen]

On 16 Mar 2012, at 12:20, Lee Essen wrote:

> 
> On 16 Mar 2012, at 12:14, Andreas Färber wrote:
> 
>> Am 16.03.2012 13:02, schrieb Lee Essen:
>>> 
>>> target_list=""
>>> 
>>> @@ -442,6 +443,7 @@ SunOS)
>>>  # have to select again, because `uname -m` returns i86pc
>>>  # even on an x86_64 box.
>>>  solariscpu=`isainfo -k`
>>> +  shell="bash"
>> 
>> Are you sure this is safe for Solaris 9+? In
>> https://bugs.launchpad.net/qemu/+bug/636315 we concluded that
>> /usr/xpg4/bin/sh were the best choice of POSIX-compliant shell provided
>> on Solaris 10. Blue's script fixes were never applied I believe…
>> 
> 
> # /usr/xpg4/bin/sh scripts/tracetool 
> scripts/tracetool: line 113: syntax error at line 126: `)' unexpected
> 
> tracetool seems to require bash … I'm happy to look at resolving that, perhaps thats a better fix.
> 

slap_wrist_for_not_looking_properly++;

It looks like a bug in tracetool (unless I completely misunderstand it), that bash doesn't care about…

# Get the format string including double quotes for a trace event
get_fmt()
{
    puts "${1#*)}"
}

So I will cancel the first two patches … and submit a fix for this instead.

Lee.

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Andreas Färber]

Am 16.03.2012 13:15, schrieb Peter Maydell:
> On 16 March 2012 12:02, Lee Essen <lee.essen@nowonline.co.uk> wrote:
>> Adds support to configure for controlling which shell to use, defaults to "sh" as before
>> but adds "bash" for Solaris/Illumos builds. Plus ensures that tracetool is called with a
>> shell.
> 
> Ugh. If we have bashisms in our shell scripts/configure/makefiles etc we should
> fix them, not paper over them.
> 
> If Solaris' /bin/sh isn't a POSIX sh that's a bug in Solaris :-)

Nah, Sun had really long support cycles and used to provide a POSIX sh
alongside their old sh for compatibility with themselves. ;-) We found
that actually documented in their man pages while investigating that in
response to my bug report. (Lee, don't forget to search the archives!)

> 
>> -echo "                           Available backends:" $("$source_path"/scripts/tracetool --list-backends)
>> +echo "                           Available backends:" $($shell "$source_path"/scripts/tracetool --list-backends)
> 
> This shouldn't be necessary -- tracetool has a #!/bin/sh at the top.
> If it needs bash then that should be fixed.

No, please. I'd be okay with setting shell="bash" in a reasonably
limited environment (say, Solaris 11) but not with requiring bash for
all platforms.

The issue here is really just getting a fully POSIX-conformant shell.
And the way I expect this to work is by executing configure and make in
such a shell and by not having hardcoded /bin/sh creep in through some
shebang line.

Andreas

>> -sh "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
>> +$shell "$source_path/scripts/tracetool" "--$trace_backend" --check-backend > /dev/null 2> /dev/null
> 
> ...and we shouldn't need to use either 'sh' or '$shell' here...
> 
> -- PMM

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Peter Maydell]

On 16 March 2012 12:24, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 16.03.2012 13:15, schrieb Peter Maydell:
>> This shouldn't be necessary -- tracetool has a #!/bin/sh at the top.
>> If it needs bash then that should be fixed.
>
> No, please. I'd be okay with setting shell="bash" in a reasonably
> limited environment (say, Solaris 11) but not with requiring bash for
> all platforms.

I meant, we should fix tracetool so that it really is a POSIX
conformant shell script, since that's what it claims to require.

> The issue here is really just getting a fully POSIX-conformant shell.
> And the way I expect this to work is by executing configure and make in
> such a shell and by not having hardcoded /bin/sh creep in through some
> shebang line.

The way I expect this to work is that /bin/sh should be a posix shell...

-- PMM

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Andreas Färber]

Am 16.03.2012 13:20, schrieb Lee Essen:
> 
> On 16 Mar 2012, at 12:14, Andreas Färber wrote:
> 
>> Am 16.03.2012 13:02, schrieb Lee Essen:
>>>
>>> +echo "SHELL=$shell" >> $config_host_mak
>>
>> Why?
>>
> 
> See patch 2/7 … there are several calls to tracetool that hardcode "sh" in the Makefile … again if
> the better solution is removing the bash requirement then this problem goes away completely.

My point is, there is already use of $(SHELL) in Makefile. So why do you
need to explicitly set it here? Such things need to be explained in the
commit message. Was the variable being used unassigned? Or are you
trying to use a non-GNU make?

The concept of using $(SHELL) in make seems valid. Dunno if there is an
easier, built-in way to execute a script in the current shell than
hardcoding some executable name that needs to be in the $PATH?

Andreas

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On 03/16/2012 06:35 AM, Peter Maydell wrote:
>> The issue here is really just getting a fully POSIX-conformant shell.
>> And the way I expect this to work is by executing configure and make in
>> such a shell and by not having hardcoded /bin/sh creep in through some
>> shebang line.
> 
> The way I expect this to work is that /bin/sh should be a posix shell...

Then your expectations are wrong.  POSIX itself says that /bin/sh need
not be the POSIX shell, and merely requires that you can query for a
conforming PATH to use, and that the 'sh' found on that PATH search is
the POSIX shell (that is, 'command -p sh' will be conforming, but won't
necessarily be /bin/sh).  This weasel-wording is intentionally written
specifically for Solaris, since they refuse to make /bin/sh
POSIX-conforming ("it might break 30-year-old legacy scripts - gasp!"),
and instead set their conforming PATH to have /usr/xpg4/bin (or these
days, /usr/xpg6/bin:/usr/xpg4/bin) prior to /bin.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH 1/5] configure to set shell type

[Peter Maydell]

On 16 March 2012 15:58, Eric Blake <eblake@redhat.com> wrote:
> On 03/16/2012 06:35 AM, Peter Maydell wrote:
>> The way I expect this to work is that /bin/sh should be a posix shell...
>
> Then your expectations are wrong.  POSIX itself says that /bin/sh need
> not be the POSIX shell, and merely requires that you can query for a
> conforming PATH to use, and that the 'sh' found on that PATH search is
> the POSIX shell (that is, 'command -p sh' will be conforming, but won't
> necessarily be /bin/sh).

Yes, well strictly POSIX says that "If the first line of a file of shell
commands starts with the characters "#!" , the results are unspecified",
so we're already in the realm of undefined behaviour if we put anything
at the top of our shell scripts.

>  This weasel-wording is intentionally written
> specifically for Solaris, since they refuse to make /bin/sh
> POSIX-conforming ("it might break 30-year-old legacy scripts - gasp!"),
> and instead set their conforming PATH to have /usr/xpg4/bin (or these
> days, /usr/xpg6/bin:/usr/xpg4/bin) prior to /bin.

Google suggests that Solaris 11 finally kicks the legacy bourne shell
out of /bin/sh...

(I'm mostly just pushing back a little at uglifying our build scripts
for the sake of a weirdo outlier if there's a different fix (eg in this
case it looks like the script really is a bash script) that's less ugly.)

-- PMM