| Message ID | 1087EBB0-5CF0-49F1-971F-EE8ADC5E06DB@nowonline.co.uk |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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.
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
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
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
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.
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
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
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(-)