[v2] build-sys: error when slirp is not found and not disabled

Commit Message

Marc-André Lureau Oct. 6, 2022, 8:33 a.m. UTC

From: Marc-André Lureau <[e-mail]>

This is an alternative configure-time solution to "[PATCH] net:
print a more actionable error when slirp is not found".

See also "If your networking is failing after updating to the latest git
version of QEMU..." ML thread.

Signed-off-by: Marc-André Lureau <[e-mail]>
---
 meson.build | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Thomas Huth Oct. 6, 2022, 8:52 a.m. UTC | #1

On 06/10/2022 10.33, [e-mail] wrote:
> From: Marc-André Lureau <[e-mail]>
> 
> This is an alternative configure-time solution to "[PATCH] net:
> print a more actionable error when slirp is not found".
> 
> See also "If your networking is failing after updating to the latest git
> version of QEMU..." ML thread.
> 
> Signed-off-by: Marc-André Lureau <[e-mail]>
> ---
>   meson.build | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 4321b8f8da..b05080b051 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
>     endif
>   endif
>   
> +# Remove this error after QEMU 8.1 has been released.
> +if not get_option('slirp').disabled() and not slirp.found()
> +  error('libslirp is not explicitely disabled and was not found. ' +
> +        'Since qemu 7.2, libslirp is no longer included as a submodule ' +

Maybe s/qemu/QEMU/

> +        'fallback, you must install it on your system or --disable-slirp.')
> +endif
> +
>   vde = not_found
>   if not get_option('vde').auto() or have_system or have_tools
>     vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],

Reviewed-by: Thomas Huth <[e-mail]>

Christian Schoenebeck Oct. 6, 2022, 9:31 a.m. UTC | #2

On Donnerstag, 6. Oktober 2022 10:33:22 CEST [e-mail] 
wrote:
> From: Marc-André Lureau <[e-mail]>
> 
> This is an alternative configure-time solution to "[PATCH] net:
> print a more actionable error when slirp is not found".
> 
> See also "If your networking is failing after updating to the latest git
> version of QEMU..." ML thread.
> 
> Signed-off-by: Marc-André Lureau <[e-mail]>
> ---
>  meson.build | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 4321b8f8da..b05080b051 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
>    endif
>  endif
> 
> +# Remove this error after QEMU 8.1 has been released.
> +if not get_option('slirp').disabled() and not slirp.found()
> +  error('libslirp is not explicitely disabled and was not found. ' +

"explicitly", except of that:

Reviewed-by: Christian Schoenebeck <[e-mail]>

> +        'Since qemu 7.2, libslirp is no longer included as a submodule ' +
> +        'fallback, you must install it on your system or --disable-slirp.')
> +endif
> +
>  vde = not_found
>  if not get_option('vde').auto() or have_system or have_tools
>    vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],

Daniel P. Berrangé Oct. 6, 2022, 9:39 a.m. UTC | #3

On Thu, Oct 06, 2022 at 12:33:22PM +0400, [e-mail] wrote:
> From: Marc-André Lureau <[e-mail]>
> 
> This is an alternative configure-time solution to "[PATCH] net:
> print a more actionable error when slirp is not found".
> 
> See also "If your networking is failing after updating to the latest git
> version of QEMU..." ML thread.
> 
> Signed-off-by: Marc-André Lureau <[e-mail]>
> ---
>  meson.build | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 4321b8f8da..b05080b051 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
>    endif
>  endif
>  
> +# Remove this error after QEMU 8.1 has been released.
> +if not get_option('slirp').disabled() and not slirp.found()
> +  error('libslirp is not explicitely disabled and was not found. ' +
> +        'Since qemu 7.2, libslirp is no longer included as a submodule ' +
> +        'fallback, you must install it on your system or --disable-slirp.')
> +endif
> +

I'm still not convinced we should be making this a fatal error, as
opposed to treating it as a warning we display at the end of meson
execution, which is what we do in other cases where we want to
alert users to something important about their build environment.


We have this for example:

  message()
  warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
  message()
  message('CPU host architecture ' + cpu + ' support is not currently maintained.')
  message('The QEMU project intends to remove support for this host CPU in')
  message('a future release if nobody volunteers to maintain it and to')
  message('provide a build host for our continuous integration setup.')
  message('configure has succeeded and you can continue to build, but')
  message('if you care about QEMU on this platform you should contact')
  message('us upstream at [e-mail].')


This is just as important to show users as the slirp case IMHO, so
it isn't clear why this approach is insufficient for slirp too.


One irritation though, is that there's no way to get this text to
display *after* meson prints the summary() data, so it is likely
scrolled off the screen.

I think 'summary()'  ought to have a way to register warning messages
that are guaranteed to be the last thing printed, with boldness.

In absence of that, we can partially mitigate this by using a custom
summary section though.

Consider this:

@@ -3936,3 +3937,20 @@ if not supported_oses.contains(targetos)
   message('if you care about QEMU on this platform you should contact')
   message('us upstream at [e-mail].')
 endif
+
+warning_info = {}
+
+# Remove this warning after QEMU 8.1 has been released.
+if not get_option('slirp').disabled() and not slirp.found()
+    warning_info += {'SLIRP': 'libslirp not present, "user" network backend will not be available'}
+    message()
+    warning('libslirp not present, "user" network backend will not be available')
+    message()
+    message('Since qemu 7.2, libslirp is no longer included as a submodule')
+    message('fallback, you must install it on your system if you require')
+    message('-netdev user / -net user to be a supported network backend')
+    message()
+endif
+
+summary(warning_info, bool_yn: true,
+        section: 'WARNINGS 

Christian Schoenebeck Oct. 6, 2022, 10:12 a.m. UTC | #4

On Donnerstag, 6. Oktober 2022 11:39:02 CEST Daniel P. Berrangé wrote:
> On Thu, Oct 06, 2022 at 12:33:22PM +0400, [e-mail] wrote:
> > From: Marc-André Lureau <[e-mail]>
> > 
> > This is an alternative configure-time solution to "[PATCH] net:
> > print a more actionable error when slirp is not found".
> > 
> > See also "If your networking is failing after updating to the latest git
> > version of QEMU..." ML thread.
> > 
> > Signed-off-by: Marc-André Lureau <[e-mail]>
> > ---
> > 
> >  meson.build | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/meson.build b/meson.build
> > index 4321b8f8da..b05080b051 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
> > 
> >    endif
> >  
> >  endif
> > 
> > +# Remove this error after QEMU 8.1 has been released.
> > +if not get_option('slirp').disabled() and not slirp.found()
> > +  error('libslirp is not explicitely disabled and was not found. ' +
> > +        'Since qemu 7.2, libslirp is no longer included as a submodule '
> > +
> > +        'fallback, you must install it on your system or
> > --disable-slirp.') +endif
> > +
> 
> I'm still not convinced we should be making this a fatal error, as
> opposed to treating it as a warning we display at the end of meson
> execution, which is what we do in other cases where we want to
> alert users to something important about their build environment.
> 
> We have this for example:
>   message()
>   warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
>   message()
>   message('CPU host architecture ' + cpu + ' support is not currently
>   maintained.') message('The QEMU project intends to remove support for
>   this host CPU in') message('a future release if nobody volunteers to
>   maintain it and to') message('provide a build host for our continuous
>   integration setup.') message('configure has succeeded and you can
>   continue to build, but') message('if you care about QEMU on this platform
>   you should contact') message('us upstream at [e-mail].')
> 
> This is just as important to show users as the slirp case IMHO, so
> it isn't clear why this approach is insufficient for slirp too.

There is a substantial difference between those two cases: the user
immediately realizes that the missing CPU arch is not available, whereas in
this missing SLIRP case it happened to myself that I was just testing a VM
(without any networking args) and scratched my head why networking on VM
stopped working without any error message at all, so I started to check my
networking, routing, VM network config, host config, found nothing, and
eventually git-bisected the issue just to find out that its because slirp has
been removed as a submodule. And I am a developer, so imagine what a regular
user might think.

And yes, with that warning it would have saved my trouble at least, but 
considering that binaries are often auto built, chances are high that they are
at first rolled out without SLIRP and hence without user networking, and hence
still causing those wondering moments to users.

> One irritation though, is that there's no way to get this text to
> display *after* meson prints the summary() data, so it is likely
> scrolled off the screen.
> 
> I think 'summary()'  ought to have a way to register warning messages
> that are guaranteed to be the last thing printed, with boldness.
> 
> In absence of that, we can partially mitigate this by using a custom
> summary section though.
> 
> Consider this:
> 
> @@ -3936,3 +3937,20 @@ if not supported_oses.contains(targetos)
> 
>    message('if you care about QEMU on this platform you should contact')
>    message('us upstream at [e-mail].')
>  
>  endif
> 
> +
> +warning_info = {}
> +
> +# Remove this warning after QEMU 8.1 has been released.
> +if not get_option('slirp').disabled() and not slirp.found()
> +    warning_info += {'SLIRP': 'libslirp not present, "user" network backend
> will not be available'} +    message()
> +    warning('libslirp not present, "user" network backend will not be
> available') +    message()
> +    message('Since qemu 7.2, libslirp is no longer included as a
> submodule')
> +    message('fallback, you must install it on your system if you require')
> +    message('-netdev user / -net user to be a supported network backend')
> +    message()
> +endif
> +
> +summary(warning_info, bool_yn: true,
> +        section: 'WARNINGS 

Daniel P. Berrangé Oct. 6, 2022, 10:26 a.m. UTC | #5

On Thu, Oct 06, 2022 at 12:12:07PM +0200, Christian Schoenebeck wrote:
> On Donnerstag, 6. Oktober 2022 11:39:02 CEST Daniel P. Berrangé wrote:
> > On Thu, Oct 06, 2022 at 12:33:22PM +0400, [e-mail] wrote:
> > > From: Marc-André Lureau <[e-mail]>
> > > 
> > > This is an alternative configure-time solution to "[PATCH] net:
> > > print a more actionable error when slirp is not found".
> > > 
> > > See also "If your networking is failing after updating to the latest git
> > > version of QEMU..." ML thread.
> > > 
> > > Signed-off-by: Marc-André Lureau <[e-mail]>
> > > ---
> > > 
> > >  meson.build | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 4321b8f8da..b05080b051 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
> > > 
> > >    endif
> > >  
> > >  endif
> > > 
> > > +# Remove this error after QEMU 8.1 has been released.
> > > +if not get_option('slirp').disabled() and not slirp.found()
> > > +  error('libslirp is not explicitely disabled and was not found. ' +
> > > +        'Since qemu 7.2, libslirp is no longer included as a submodule '
> > > +
> > > +        'fallback, you must install it on your system or
> > > --disable-slirp.') +endif
> > > +
> > 
> > I'm still not convinced we should be making this a fatal error, as
> > opposed to treating it as a warning we display at the end of meson
> > execution, which is what we do in other cases where we want to
> > alert users to something important about their build environment.
> > 
> > We have this for example:
> >   message()
> >   warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
> >   message()
> >   message('CPU host architecture ' + cpu + ' support is not currently
> >   maintained.') message('The QEMU project intends to remove support for
> >   this host CPU in') message('a future release if nobody volunteers to
> >   maintain it and to') message('provide a build host for our continuous
> >   integration setup.') message('configure has succeeded and you can
> >   continue to build, but') message('if you care about QEMU on this platform
> >   you should contact') message('us upstream at [e-mail].')
> > 
> > This is just as important to show users as the slirp case IMHO, so
> > it isn't clear why this approach is insufficient for slirp too.
> 
> There is a substantial difference between those two cases: the user
> immediately realizes that the missing CPU arch is not available, whereas in
> this missing SLIRP case it happened to myself that I was just testing a VM
> (without any networking args) and scratched my head why networking on VM
> stopped working without any error message at all, so I started to check my
> networking, routing, VM network config, host config, found nothing, and
> eventually git-bisected the issue just to find out that its because slirp has
> been removed as a submodule. And I am a developer, so imagine what a regular
> user might think.
> 
> And yes, with that warning it would have saved my trouble at least, but 
> considering that binaries are often auto built, chances are high that they are
> at first rolled out without SLIRP and hence without user networking, and hence
> still causing those wondering moments to users.
> 
> > One irritation though, is that there's no way to get this text to
> > display *after* meson prints the summary() data, so it is likely
> > scrolled off the screen.
> > 
> > I think 'summary()'  ought to have a way to register warning messages
> > that are guaranteed to be the last thing printed, with boldness.
> > 
> > In absence of that, we can partially mitigate this by using a custom
> > summary section though.
> > 
> > Consider this:
> > 
> > @@ -3936,3 +3937,20 @@ if not supported_oses.contains(targetos)
> > 
> >    message('if you care about QEMU on this platform you should contact')
> >    message('us upstream at [e-mail].')
> >  
> >  endif
> > 
> > +
> > +warning_info = {}
> > +
> > +# Remove this warning after QEMU 8.1 has been released.
> > +if not get_option('slirp').disabled() and not slirp.found()
> > +    warning_info += {'SLIRP': 'libslirp not present, "user" network backend
> > will not be available'} +    message()
> > +    warning('libslirp not present, "user" network backend will not be
> > available') +    message()
> > +    message('Since qemu 7.2, libslirp is no longer included as a
> > submodule')
> > +    message('fallback, you must install it on your system if you require')
> > +    message('-netdev user / -net user to be a supported network backend')
> > +    message()
> > +endif
> > +
> > +summary(warning_info, bool_yn: true,
> > +        section: 'WARNINGS 

Christian Schoenebeck Oct. 6, 2022, 11:08 a.m. UTC | #6

On Donnerstag, 6. Oktober 2022 12:26:50 CEST Daniel P. Berrangé wrote:
> On Thu, Oct 06, 2022 at 12:12:07PM +0200, Christian Schoenebeck wrote:
> > On Donnerstag, 6. Oktober 2022 11:39:02 CEST Daniel P. Berrangé wrote:
> > > On Thu, Oct 06, 2022 at 12:33:22PM +0400, [e-mail] 
wrote:
> > > > From: Marc-André Lureau <[e-mail]>
> > > > 
> > > > This is an alternative configure-time solution to "[PATCH] net:
> > > > print a more actionable error when slirp is not found".
> > > > 
> > > > See also "If your networking is failing after updating to the latest
> > > > git
> > > > version of QEMU..." ML thread.
> > > > 
> > > > Signed-off-by: Marc-André Lureau <[e-mail]>
> > > > ---
> > > > 
> > > >  meson.build | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/meson.build b/meson.build
> > > > index 4321b8f8da..b05080b051 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -690,6 +690,13 @@ if not get_option('slirp').auto() or have_system
> > > > 
> > > >    endif
> > > >  
> > > >  endif
> > > > 
> > > > +# Remove this error after QEMU 8.1 has been released.
> > > > +if not get_option('slirp').disabled() and not slirp.found()
> > > > +  error('libslirp is not explicitely disabled and was not found. ' +
> > > > +        'Since qemu 7.2, libslirp is no longer included as a
> > > > submodule '
> > > > +
> > > > +        'fallback, you must install it on your system or
> > > > --disable-slirp.') +endif
> > > > +
> > > 
> > > I'm still not convinced we should be making this a fatal error, as
> > > opposed to treating it as a warning we display at the end of meson
> > > execution, which is what we do in other cases where we want to
> > > alert users to something important about their build environment.
> > > 
> > > We have this for example:
> > >   message()
> > >   warning('SUPPORT FOR THIS HOST CPU WILL GO AWAY IN FUTURE RELEASES!')
> > >   message()
> > >   message('CPU host architecture ' + cpu + ' support is not currently
> > >   maintained.') message('The QEMU project intends to remove support for
> > >   this host CPU in') message('a future release if nobody volunteers to
> > >   maintain it and to') message('provide a build host for our continuous
> > >   integration setup.') message('configure has succeeded and you can
> > >   continue to build, but') message('if you care about QEMU on this
> > >   platform
> > >   you should contact') message('us upstream at [e-mail].')
> > > 
> > > This is just as important to show users as the slirp case IMHO, so
> > > it isn't clear why this approach is insufficient for slirp too.
> > 
> > There is a substantial difference between those two cases: the user
> > immediately realizes that the missing CPU arch is not available, whereas
> > in
> > this missing SLIRP case it happened to myself that I was just testing a VM
> > (without any networking args) and scratched my head why networking on VM
> > stopped working without any error message at all, so I started to check my
> > networking, routing, VM network config, host config, found nothing, and
> > eventually git-bisected the issue just to find out that its because slirp
> > has been removed as a submodule. And I am a developer, so imagine what a
> > regular user might think.
> > 
> > And yes, with that warning it would have saved my trouble at least, but
> > considering that binaries are often auto built, chances are high that they
> > are at first rolled out without SLIRP and hence without user networking,
> > and hence still causing those wondering moments to users.
> > 
> > > One irritation though, is that there's no way to get this text to
> > > display *after* meson prints the summary() data, so it is likely
> > > scrolled off the screen.
> > > 
> > > I think 'summary()'  ought to have a way to register warning messages
> > > that are guaranteed to be the last thing printed, with boldness.
> > > 
> > > In absence of that, we can partially mitigate this by using a custom
> > > summary section though.
> > > 
> > > Consider this:
> > > 
> > > @@ -3936,3 +3937,20 @@ if not supported_oses.contains(targetos)
> > > 
> > >    message('if you care about QEMU on this platform you should contact')
> > >    message('us upstream at [e-mail].')
> > >  
> > >  endif
> > > 
> > > +
> > > +warning_info = {}
> > > +
> > > +# Remove this warning after QEMU 8.1 has been released.
> > > +if not get_option('slirp').disabled() and not slirp.found()
> > > +    warning_info += {'SLIRP': 'libslirp not present, "user" network
> > > backend will not be available'} +    message()
> > > +    warning('libslirp not present, "user" network backend will not be
> > > available') +    message()
> > > +    message('Since qemu 7.2, libslirp is no longer included as a
> > > submodule')
> > > +    message('fallback, you must install it on your system if you
> > > require')
> > > +    message('-netdev user / -net user to be a supported network
> > > backend')
> > > +    message()
> > > +endif
> > > +
> > > +summary(warning_info, bool_yn: true,
> > > +        section: 'WARNINGS 

Patch

diff --git a/meson.build b/meson.build
index 4321b8f8da..b05080b051 100644
--- a/meson.build
+++ b/meson.build
@@ -690,6 +690,13 @@  if not get_option('slirp').auto() or have_system
   endif
 endif
 
+# Remove this error after QEMU 8.1 has been released.
+if not get_option('slirp').disabled() and not slirp.found()
+  error('libslirp is not explicitely disabled and was not found. ' +
+        'Since qemu 7.2, libslirp is no longer included as a submodule ' +
+        'fallback, you must install it on your system or --disable-slirp.')
+endif
+
 vde = not_found
 if not get_option('vde').auto() or have_system or have_tools
   vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],