| Message ID | 20170327201146.1820-1-sw@weilnetz.de |
|---|---|
| State | Under Review |
| Headers | show |
On 27 March 2017 at 21:11, Stefan Weil <sw@weilnetz.de> wrote: > This check had several problems which are fixed here: > > * Calling "configure --help" was no longer possible on Cygwin. > Fix this by introducing a third state for supported_os and > by moving the error handling code. > Move the error handling code for supported_cpu, too. > > * Fix the error text. Use target cpu and target os because the > build cpu and build host don't matter here. I think it is important that we print the deprecation message last, after all the other configure output. Otherwise it will scroll off the screen and most people will miss it. That's why I put it where it is. > * Support cross compilation with the most common cross prefixes > for Mingw-w64. Other cross builds are still broken! > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > This fixes a regression: building QEMU for Windows with a cross > build under Cygwin is currently broken. > > Can the patch be applied directly, or who should send a pull request? > > Stefan > > configure | 68 +++++++++++++++++++++++++++++++++++---------------------------- > 1 file changed, 38 insertions(+), 30 deletions(-) > > diff --git a/configure b/configure > index d1ce33bc79..902bd20070 100755 > --- a/configure > +++ b/configure > @@ -322,7 +322,7 @@ jemalloc="no" > replication="yes" > > supported_cpu="no" > -supported_os="no" > +supported_os="deprecated" I don't see any need for anything other than 'yes' or 'no' ? > > # parse CC options first > for opt do > @@ -440,7 +440,13 @@ int main(void) { return 0; } > EOF > } > > -if check_define __linux__ ; then > +if test "$cross_prefix" = "i686_64-w64-mingw32-" ; then > + targetos='MINGW32' > +elif test "$cross_prefix" = "x86_64-w64-mingw32-" ; then > + targetos='MINGW32' > +elif test -n "$cross_prefix" ; then > + targetos="$cross_prefix" This doesn't look right. We already have too many cases where we determine the target OS by something other than looking at check_defines; one thing I'd like to do when we've got rid of unsupported OSes is to make sure that we determine the target OS by check_define and only by check_define. > +elif check_define __linux__ ; then > targetos="Linux" > elif check_define _WIN32 ; then > targetos='MINGW32' > @@ -694,7 +700,7 @@ Linux) > supported_os="yes" > ;; > *) > - error_exit "Unsupported host OS $targetos" > + supported_os="no" This was deliberately an error_exit because anything going down that path got the Linux defines/includes by accident and we thought that would not work on anything else. What was using it? If we need an extra entry in the case statement we can add one. > ;; > esac > > @@ -1428,6 +1434,34 @@ EOF > exit 0 > fi > config_host_mak="config-host.mak" > > echo "# Automatically generated by configure - do not modify" >config-all-disas.mak > @@ -5212,7 +5220,7 @@ if test "$mingw32" = "yes" ; then > echo "CONFIG_QGA_NTDDDISK=y" >> $config_host_mak > fi > if test "$guest_agent_msi" = "yes"; then > - echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak > + echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak > echo "QEMU_GA_MSI_MINGW_DLL_PATH=${QEMU_GA_MSI_MINGW_DLL_PATH}" >> $config_host_mak > echo "QEMU_GA_MSI_WITH_VSS=${QEMU_GA_MSI_WITH_VSS}" >> $config_host_mak > echo "QEMU_GA_MSI_ARCH=${QEMU_GA_MSI_ARCH}" >> $config_host_mak Stray unintended change? > -- > 2.11.0 > thanks -- PMM
On 27/03/2017 22:11, Stefan Weil wrote: > * Support cross compilation with the most common cross prefixes > for Mingw-w64. Other cross builds are still broken! Can you explain how it's broken? Why does check_define not work at this point, for cross builds? "../configure --cross-prefix=x86_64-w64-mingw32-" seems to work for me. Paolo > +if test "$cross_prefix" = "i686_64-w64-mingw32-" ; then > + targetos='MINGW32' > +elif test "$cross_prefix" = "x86_64-w64-mingw32-" ; then > + targetos='MINGW32' > +elif test -n "$cross_prefix" ; then > + targetos="$cross_prefix" > +elif check_define __linux__ ; then > targetos="Linux"
Am 28.03.2017 um 10:04 schrieb Paolo Bonzini: > On 27/03/2017 22:11, Stefan Weil wrote: >> * Support cross compilation with the most common cross prefixes >> for Mingw-w64. Other cross builds are still broken! > Can you explain how it's broken? Why does check_define not work at this > point, for cross builds? > > "../configure --cross-prefix=x86_64-w64-mingw32-" seems to work for me. > > Paolo You are right. I was wrong because of this use case: $ ./configure '--enable-debug' '--cross-prefix=x86_64-w64-mingw32' ERROR: Unsupported host OS CYGWIN_NT-6.1 So the error message is misleading when I specify a wrong cross prefix (it should have been x86_64-w64-mingw32- instead of x86_64-w64-mingw32). With the correct cross prefix, builds work, so cross builds are not a critical issue as I thought. The second issue which remains is calling ./configure --help: $ ./configure --help ERROR: Unsupported host OS CYGWIN_NT-6.1 The third issue is the message for deprecated cpus or targets which should use "target" instead of "host". All these three issues are not critical for the next QEMU version. Therefore I think they can be fixed after the release. Regards Stefan
On 28 March 2017 at 12:53, Stefan Weil <sw@weilnetz.de> wrote: > Am 28.03.2017 um 10:04 schrieb Paolo Bonzini: >> On 27/03/2017 22:11, Stefan Weil wrote: >>> * Support cross compilation with the most common cross prefixes >>> for Mingw-w64. Other cross builds are still broken! >> Can you explain how it's broken? Why does check_define not work at this >> point, for cross builds? >> >> "../configure --cross-prefix=x86_64-w64-mingw32-" seems to work for me. >> >> Paolo > > You are right. I was wrong because of this use case: > > $ ./configure '--enable-debug' '--cross-prefix=x86_64-w64-mingw32' > > ERROR: Unsupported host OS CYGWIN_NT-6.1 > > So the error message is misleading when I specify a wrong cross prefix > (it should have been x86_64-w64-mingw32- instead of x86_64-w64-mingw32). Ah, I see. In the fall-through case we'd then complain that the compiler didn't work. I think the underlying problem here is that we use the compiler (for the check_prefix tests) before the "does this compiler even work" test... (This only happens in the case where you're doing a cross compile and the build machine is not a supported QEMU host.) > With the correct cross prefix, builds work, so cross builds are > not a critical issue as I thought. > > The second issue which remains is calling ./configure --help: > > $ ./configure --help > > ERROR: Unsupported host OS CYGWIN_NT-6.1 Yeah, we should not complain for --help; I'll see if I can fix that. > The third issue is the message for deprecated cpus or targets > which should use "target" instead of "host". We are not (yet) deprecating any targets, so we don't want to say "target". What we're deprecating are hosts. (In a cross compile setup the 'host' (machine being compiled for) isn't the 'build' (machine running the compiler), but I think if you're advanced enough to be cross-compiling you can deal with the confusing terminology.) thanks -- PMM
On 28/03/17 12:53, Stefan Weil wrote: > You are right. I was wrong because of this use case: > > $ ./configure '--enable-debug' '--cross-prefix=x86_64-w64-mingw32' > > ERROR: Unsupported host OS CYGWIN_NT-6.1 > > So the error message is misleading when I specify a wrong cross prefix > (it should have been x86_64-w64-mingw32- instead of x86_64-w64-mingw32). > > With the correct cross prefix, builds work, so cross builds are > not a critical issue as I thought. > > The second issue which remains is calling ./configure --help: > > $ ./configure --help > > ERROR: Unsupported host OS CYGWIN_NT-6.1 > > The third issue is the message for deprecated cpus or targets > which should use "target" instead of "host". > > All these three issues are not critical for the next QEMU version. > Therefore I think they can be fixed after the release. On a related note, I was scratching my head for a while over the weekend trying to build on MingW with the "ERROR: Unsupported host ..." message, since it also pops up if you don't have gcc installed, or indeed gcc but not the gcc headers installed. The latter case was much harder to spot since config.log showed other warnings and #errors from previous configure tests before failing with the unrelated ERROR message. ATB, Mark.
Am 28.03.2017 um 09:10 schrieb Peter Maydell: > On 27 March 2017 at 21:11, Stefan Weil <sw@weilnetz.de> wrote: >> This check had several problems which are fixed here: >> >> * Calling "configure --help" was no longer possible on Cygwin. >> Fix this by introducing a third state for supported_os and >> by moving the error handling code. >> Move the error handling code for supported_cpu, too. >> >> * Fix the error text. Use target cpu and target os because the >> build cpu and build host don't matter here. > I think it is important that we print the deprecation > message last, after all the other configure output. > Otherwise it will scroll off the screen and most people > will miss it. That's why I put it where it is. Ok, then only the error handling for unsupported target os has to be moved (to support ./configure --help). >> index d1ce33bc79..902bd20070 100755 >> --- a/configure >> +++ b/configure >> @@ -322,7 +322,7 @@ jemalloc="no" >> replication="yes" >> >> supported_cpu="no" >> -supported_os="no" >> +supported_os="deprecated" > I don't see any need for anything other than 'yes' or 'no' ? yes = file no = raise error deprecated = raise warning Needed for delayed error handling, see below. >> # parse CC options first >> for opt do >> @@ -440,7 +440,13 @@ int main(void) { return 0; } >> EOF >> } >> >> -if check_define __linux__ ; then >> +if test "$cross_prefix" = "i686_64-w64-mingw32-" ; then >> + targetos='MINGW32' >> +elif test "$cross_prefix" = "x86_64-w64-mingw32-" ; then >> + targetos='MINGW32' >> +elif test -n "$cross_prefix" ; then >> + targetos="$cross_prefix" > This doesn't look right. We already have too many > cases where we determine the target OS by something > other than looking at check_defines; one thing > I'd like to do when we've got rid of unsupported > OSes is to make sure that we determine the target > OS by check_define and only by check_define. That part of my patch is not needed. I added it because I did not realize that "check_define" already does the correct thing and because a wrong cross prefix raises a misleading error message. >> +elif check_define __linux__ ; then >> targetos="Linux" >> elif check_define _WIN32 ; then >> targetos='MINGW32' >> @@ -694,7 +700,7 @@ Linux) >> supported_os="yes" >> ;; >> *) >> - error_exit "Unsupported host OS $targetos" >> + supported_os="no" > This was deliberately an error_exit because anything > going down that path got the Linux defines/includes by > accident and we thought that would not work on > anything else. What was using it? If we need an extra > entry in the case statement we can add one. It remains an error (that's why I introduced a third state for supported_os), but must be handled later, after processing a potential --help option. >> echo "CONFIG_QGA_NTDDDISK=y" >> $config_host_mak >> fi >> if test "$guest_agent_msi" = "yes"; then >> - echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak >> + echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak >> echo "QEMU_GA_MSI_MINGW_DLL_PATH=${QEMU_GA_MSI_MINGW_DLL_PATH}" >> $config_host_mak >> echo "QEMU_GA_MSI_WITH_VSS=${QEMU_GA_MSI_WITH_VSS}" >> $config_host_mak >> echo "QEMU_GA_MSI_ARCH=${QEMU_GA_MSI_ARCH}" >> $config_host_mak > Stray unintended change? Whitespace at line ending removed by my editor. Stefan
On 28 March 2017 at 13:07, Stefan Weil <sw@weilnetz.de> wrote: > Am 28.03.2017 um 09:10 schrieb Peter Maydell: >> On 27 March 2017 at 21:11, Stefan Weil <sw@weilnetz.de> wrote: >>> *) >>> - error_exit "Unsupported host OS $targetos" >>> + supported_os="no" >> This was deliberately an error_exit because anything >> going down that path got the Linux defines/includes by >> accident and we thought that would not work on >> anything else. What was using it? If we need an extra >> entry in the case statement we can add one. > > It remains an error (that's why I introduced a third state for > supported_os), but must be handled later, after processing > a potential --help option. I think the right fix for this is to special case --help to bypass more checks and to happen earlier. For instance if you don't happen to have a 'python' on your PATH then we do this: netbsdvm# ./configure --help ERROR: Python not found. Use --python=/path/to/python I'll have a go at writing a patch. thanks -- PMM
diff --git a/configure b/configure index d1ce33bc79..902bd20070 100755 --- a/configure +++ b/configure @@ -322,7 +322,7 @@ jemalloc="no" replication="yes" supported_cpu="no" -supported_os="no" +supported_os="deprecated" # parse CC options first for opt do @@ -440,7 +440,13 @@ int main(void) { return 0; } EOF } -if check_define __linux__ ; then +if test "$cross_prefix" = "i686_64-w64-mingw32-" ; then + targetos='MINGW32' +elif test "$cross_prefix" = "x86_64-w64-mingw32-" ; then + targetos='MINGW32' +elif test -n "$cross_prefix" ; then + targetos="$cross_prefix" +elif check_define __linux__ ; then targetos="Linux" elif check_define _WIN32 ; then targetos='MINGW32' @@ -694,7 +700,7 @@ Linux) supported_os="yes" ;; *) - error_exit "Unsupported host OS $targetos" + supported_os="no" ;; esac @@ -1428,6 +1434,34 @@ EOF exit 0 fi +if test "$supported_cpu" = "no"; then + echo "WARNING: SUPPORT FOR THIS TARGET CPU WILL GO AWAY IN FUTURE RELEASES!" + echo + echo "CPU target architecture $cpu support is not currently maintained." + echo "The QEMU project intends to remove support for this target CPU in" + echo "a future release if nobody volunteers to maintain it and to" + echo "provide a build host for our continuous integration setup." + echo "configure has succeeded and you can continue to build, but" + echo "if you care about QEMU on this platform you should contact" + echo "us upstream at qemu-devel@nongnu.org." + echo +fi + +if test "$supported_os" = "no"; then + error_exit "Unsupported target OS $targetos" +elif test "$supported_os" = "deprecated"; then + echo "WARNING: SUPPORT FOR THIS TARGET OS WILL GO AWAY IN FUTURE RELEASES!" + echo + echo "Target OS $targetos support is not currently maintained." + echo "The QEMU project intends to remove support for this target OS in" + echo "a future release if nobody volunteers to maintain it and to" + echo "provide a build host for our continuous integration setup." + echo "configure has succeeded and you can continue to build, but" + echo "if you care about QEMU on this platform you should contact" + echo "us upstream at qemu-devel@nongnu.org." + echo +fi + # Now we have handled --enable-tcg-interpreter and know we're not just # printing the help message, bail out if the host CPU isn't supported. if test "$ARCH" = "unknown"; then @@ -5127,32 +5161,6 @@ if test "$sdl_too_old" = "yes"; then echo "-> Your SDL version is too old - please upgrade to have SDL support" fi -if test "$supported_cpu" = "no"; then - echo - echo "WARNING: SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!" - echo - echo "CPU host architecture $cpu support is not currently maintained." - echo "The QEMU project intends to remove support for this host CPU in" - echo "a future release if nobody volunteers to maintain it and to" - echo "provide a build host for our continuous integration setup." - echo "configure has succeeded and you can continue to build, but" - echo "if you care about QEMU on this platform you should contact" - echo "us upstream at qemu-devel@nongnu.org." -fi - -if test "$supported_os" = "no"; then - echo - echo "WARNING: SUPPORT FOR THIS HOST OS WILL GO AWAY IN FUTURE RELEASES!" - echo - echo "Host OS $targetos support is not currently maintained." - echo "The QEMU project intends to remove support for this host OS in" - echo "a future release if nobody volunteers to maintain it and to" - echo "provide a build host for our continuous integration setup." - echo "configure has succeeded and you can continue to build, but" - echo "if you care about QEMU on this platform you should contact" - echo "us upstream at qemu-devel@nongnu.org." -fi - config_host_mak="config-host.mak" echo "# Automatically generated by configure - do not modify" >config-all-disas.mak @@ -5212,7 +5220,7 @@ if test "$mingw32" = "yes" ; then echo "CONFIG_QGA_NTDDDISK=y" >> $config_host_mak fi if test "$guest_agent_msi" = "yes"; then - echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak + echo "QEMU_GA_MSI_ENABLED=yes" >> $config_host_mak echo "QEMU_GA_MSI_MINGW_DLL_PATH=${QEMU_GA_MSI_MINGW_DLL_PATH}" >> $config_host_mak echo "QEMU_GA_MSI_WITH_VSS=${QEMU_GA_MSI_WITH_VSS}" >> $config_host_mak echo "QEMU_GA_MSI_ARCH=${QEMU_GA_MSI_ARCH}" >> $config_host_mak
This check had several problems which are fixed here: * Calling "configure --help" was no longer possible on Cygwin. Fix this by introducing a third state for supported_os and by moving the error handling code. Move the error handling code for supported_cpu, too. * Fix the error text. Use target cpu and target os because the build cpu and build host don't matter here. * Support cross compilation with the most common cross prefixes for Mingw-w64. Other cross builds are still broken! Signed-off-by: Stefan Weil <sw@weilnetz.de> --- This fixes a regression: building QEMU for Windows with a cross build under Cygwin is currently broken. Can the patch be applied directly, or who should send a pull request? Stefan configure | 68 +++++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 30 deletions(-)