[PATCH] meson.build: Don't look for libudev for static builds

[PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

commit f01496a314d916 moved the logic for detecting libudev from
configure to meson.build, but in the process it dropped the condition
that meant we only ask pkg-config about libudev for a non-static
build.

This breaks static builds of the system emulators on at least Ubuntu
18.04.4, because on that host there is no static libudev but
pkg-config still claims it exists.

Reinstate the logic that we had in the configure check.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We could certainly do something cleverer here, but basic "convert
from configure to meson" should in general not also be changing the
detection logic IMHO.  We can make the logic smarter as a follow-on
patch if desired.
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 3161c1f037a..07da66e1d81 100644
--- a/meson.build
+++ b/meson.build
@@ -271,7 +271,7 @@ if 'CONFIG_CURL' in config_host
                             link_args: config_host['CURL_LIBS'].split())
 endif
 libudev = not_found
-if targetos == 'linux' and (have_system or have_tools)
+if targetos == 'linux' and (have_system or have_tools) and not enable_static
   libudev = dependency('libudev',
                        required: get_option('mpath').enabled(),
                        static: enable_static)
-- 
2.20.1

Re: [PATCH] meson.build: Don't look for libudev for static builds

[罗勇刚(Yonggang Luo)]

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

So the better way is pkg-config handling sttaic properly？
On Fri, Oct 2, 2020 at 6:53 PM Peter Maydell <peter.maydell@linaro.org>
wrote:
>
> commit f01496a314d916 moved the logic for detecting libudev from
> configure to meson.build, but in the process it dropped the condition
> that meant we only ask pkg-config about libudev for a non-static
> build.
>
> This breaks static builds of the system emulators on at least Ubuntu
> 18.04.4, because on that host there is no static libudev but
> pkg-config still claims it exists.
>
> Reinstate the logic that we had in the configure check.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> We could certainly do something cleverer here, but basic "convert
> from configure to meson" should in general not also be changing the
> detection logic IMHO.  We can make the logic smarter as a follow-on
> patch if desired.
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 3161c1f037a..07da66e1d81 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -271,7 +271,7 @@ if 'CONFIG_CURL' in config_host
>                              link_args: config_host['CURL_LIBS'].split())
>  endif
>  libudev = not_found
> -if targetos == 'linux' and (have_system or have_tools)
> +if targetos == 'linux' and (have_system or have_tools) and not
enable_static
>    libudev = dependency('libudev',
>                         required: get_option('mpath').enabled(),
>                         static: enable_static)
> --
> 2.20.1
>
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 2128 bytes --]

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 12:52, Peter Maydell wrote:
> commit f01496a314d916 moved the logic for detecting libudev from
> configure to meson.build, but in the process it dropped the condition
> that meant we only ask pkg-config about libudev for a non-static
> build.
> 
> This breaks static builds of the system emulators on at least Ubuntu
> 18.04.4, because on that host there is no static libudev but
> pkg-config still claims it exists.
> 
> Reinstate the logic that we had in the configure check.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I don't think this is a good idea.  Having a completely new build system
also lets us notice all these weird one-off tests, decide whether they
are worth being kept (like the SDL -Wno-undef workaround) or not, and
possibly come up with a policy that avoids having to make one-off decisions.

If "../configure --static" worked and now doesn't then it would be a
clear regression, but a one off check should have a bigger justification
than "39 --disable switches have worked so far and 39 < 40".

Here are three alternatives to the patch:

- the workaround: just leave things as they are and add
--disable-libudev to your script.  We have just added a month ago a
completely new dependency that would have the same issue (libu2f); we
didn't add "test $static" just because you don't have libu2f installed
and therefore you didn't notice.  The day you updated your system ended
up with libu2f-dev installed, you would probably just add
--disable-libu2f instead of adding a test for $static.  So why should
libudev be treated differently?

- the cheap fix: deprecate static building of emulators in the configure
script (in the meanwhile you add --disable-libudev); if people complain,
we figure out the dependencies that they care about and we try to
understand if it's actually possible to build a statically linked
emulator for their usecase.

- the complicated fix: check which statically linked libraries are
available in e.g. Debian and disable _all_ other dependencies with
--static (most of them will be the ones that you already have to disable
 in your script, and most of them will be in configure).  Based on the
outcome, decide whether it's actually possible to build a statically
linked emulator that makes sense.

I wouldn't be surprised if people trying to statically build emulators
are getting worse performance just due to a missing static libaio or
libio_uring, for example, and these people might even be building a
statically-linked QEMU for use with KVM (like Firecracker), i.e. they
would care about performance.

Finally, no matter how we proceed, static building of system emulators
is not covered by either CI or tests/docker (only user-mode emulators
are).  Even if we deprecate it, we should make sure that your
configuration is reproduced in either Travis or GitLab, otherwise people
will keep on breaking it.  That would also document that building a
statically-linked system emulator is messy and requires a few dozen
--disable options.

Thanks,

Paolo

> We could certainly do something cleverer here, but basic "convert
> from configure to meson" should in general not also be changing the
> detection logic IMHO.  We can make the logic smarter as a follow-on
> patch if desired.
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 3161c1f037a..07da66e1d81 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -271,7 +271,7 @@ if 'CONFIG_CURL' in config_host
>                              link_args: config_host['CURL_LIBS'].split())
>  endif
>  libudev = not_found
> -if targetos == 'linux' and (have_system or have_tools)
> +if targetos == 'linux' and (have_system or have_tools) and not enable_static
>    libudev = dependency('libudev',
>                         required: get_option('mpath').enabled(),
>                         static: enable_static)
> 

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 13:15, 罗勇刚(Yonggang Luo) wrote:
> So the better way is pkg-config handling sttaic properly？

The problem is that you cannot handle it properly.  Consider for example
libmultipath, which requires the program to define a couple functions
for static linking to work.  A compile-time check that the library works
would fail, and therefore Meson (and configure) are just punting and
declaring it a user configuration issue.  Meson will warn about it, but
it will proceed anyway.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 13:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/10/20 12:52, Peter Maydell wrote:
> > commit f01496a314d916 moved the logic for detecting libudev from
> > configure to meson.build, but in the process it dropped the condition
> > that meant we only ask pkg-config about libudev for a non-static
> > build.
> >
> > This breaks static builds of the system emulators on at least Ubuntu
> > 18.04.4, because on that host there is no static libudev but
> > pkg-config still claims it exists.
> >
> > Reinstate the logic that we had in the configure check.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> I don't think this is a good idea.  Having a completely new build system
> also lets us notice all these weird one-off tests, decide whether they
> are worth being kept (like the SDL -Wno-undef workaround) or not, and
> possibly come up with a policy that avoids having to make one-off decisions.
>
> If "../configure --static" worked and now doesn't then it would be a
> clear regression, but a one off check should have a bigger justification
> than "39 --disable switches have worked so far and 39 < 40".

My configure command line used to work and now it doesn't.
There is no workaround that I'm aware of that I can use
by tweaking the configure options. That's a clear regression.

> Here are three alternatives to the patch:
>
> - the workaround: just leave things as they are and add
> --disable-libudev to your script.

There is no --disable-udev ! If there was I'd just have
used it.

e104462:bionic:static-sys$  '../../configure'
'--target-list=arm-softmmu' '--enable-debug' '--static'
'--disable-tools' '--disable-sdl' '--disable-gtk' '--disable-vnc'
'--disable-virtfs' '--disable-attr' '--disable-libiscsi'
'--disable-libnfs' '--disable-libusb' '--disable-opengl'
'--disable-numa' '--disable-usb-redir' '--disable-bzip2'
'--audio-drv-list=' '--disable-guest-agent' '--disable-vte'
--disable-udev
ERROR: unknown option --disable-udev
Try '../../configure --help' for more information

> We have just added a month ago a
> completely new dependency that would have the same issue (libu2f); we
> didn't add "test $static" just because you don't have libu2f installed
> and therefore you didn't notice.  The day you updated your system ended
> up with libu2f-dev installed, you would probably just add
> --disable-libu2f instead of adding a test for $static.  So why should
> libudev be treated differently?

The last time this came up it was in an all-in-configure
test, so I took the approach of making the test a bit smarter
to sanity check that what it was getting from pkg-config
really worked:
https://patchew.org/QEMU/20200928160402.7961-1-peter.maydell@linaro.org/
I don't know enough meson to do that in meson, so this patch is
the simple change that fixes the regression by reinstating the
logic configure had.

> - the complicated fix: check which statically linked libraries are
> available in e.g. Debian and disable _all_ other dependencies with
> --static (most of them will be the ones that you already have to disable
>  in your script, and most of them will be in configure).  Based on the
> outcome, decide whether it's actually possible to build a statically
> linked emulator that makes sense.

I don't think we want to try to say "look at what statically
linked libraries are in Debian". The point of configure
style logic is to look at what is present when we try to compile
and, when something is an optional feature, only compile it
in if it's going to work.

It would be better to do the "see if a static library is present"
test. This isn't too hard to do in configure (compare that
six line fix to the detection of libgio). Hopefully it is
not too hard to do in meson ?

> Finally, no matter how we proceed, static building of system emulators
> is not covered by either CI or tests/docker (only user-mode emulators
> are).  Even if we deprecate it, we should make sure that your
> configuration is reproduced in either Travis or GitLab, otherwise people
> will keep on breaking it.  That would also document that building a
> statically-linked system emulator is messy and requires a few dozen
> --disable options.

I agree that we don't really very solidly support this use case,
and it's pretty much "if it doesn't work we accept point-fixes
to the build machinery and/or recommend more use of --disable-foo".
But it is still useful sometimes to have.

thanks
-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 2 Oct 2020 at 13:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > - the workaround: just leave things as they are and add
> > --disable-libudev to your script.
>
> There is no --disable-udev !

...and there's no --disable-libudev either :-)

-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 14:36, Peter Maydell wrote:
> On Fri, 2 Oct 2020 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 2 Oct 2020 at 13:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> - the workaround: just leave things as they are and add
>>> --disable-libudev to your script.
>>
>> There is no --disable-udev !
> 
> ...and there's no --disable-libudev either :-)

You're right---I was sure I used some get_option but it's not
--disable-libudev; it's --disable-mpath.  That made sense when mpath was
the only thing that used libudev, but probably now we should add a
separate --disable-libudev.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 13:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/10/20 14:36, Peter Maydell wrote:
> > On Fri, 2 Oct 2020 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Fri, 2 Oct 2020 at 13:02, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>> - the workaround: just leave things as they are and add
> >>> --disable-libudev to your script.
> >>
> >> There is no --disable-udev !
> >
> > ...and there's no --disable-libudev either :-)
>
> You're right---I was sure I used some get_option but it's not
> --disable-libudev; it's --disable-mpath.  That made sense when mpath was
> the only thing that used libudev, but probably now we should add a
> separate --disable-libudev.

Well, this gets into the question of what --disable-foo options
should be doing. They're user facing, so really they ought to be
for user-facing features. So for instance:
  seccomp         seccomp support
  libssh          ssh block device support
  dmg             dmg image format support
  gtk             gtk UI

are all enabling or disabling features the user might plausibly
have an opinion about whether they want or not.

These are bit more dubious -- they are enabling/disabling
a specific library, and so the help text has had to do
the translation of that for the user into "what actual
feature in QEMU am I losing here if I disable this?";
  libxml2         for Parallels image format
  libusb          libusb (for usb passthrough)

and then these give the user no hints at all:
  libpmem         libpmem support
  libdaxctl       libdaxctl support

so it's not clear what they're intended for (unless it's
solely for the niche use case of "work around the build
machinery not correctly detecting whether the library is
available"...)

Having a --disable-libudev seems to me strictly worse
than either
 (a) the build system actually being able to correctly
identify that the host has a libudev available to link to
 (b) having an option with a name and help text that tells
the user what they're actually gaining or losing by the
feature being enabled/disabled

thanks
-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 14:35, Peter Maydell wrote:
> 
> It would be better to do the "see if a static library is present"
> test. This isn't too hard to do in configure (compare that
> six line fix to the detection of libgio). Hopefully it is
> not too hard to do in meson ?

Yes, something like:

if enable_static
  skeleton = 'int main(void) { return 0; }'
  if not cc.links(skeleton, dependencies: libudev)
    if get_option('mpath').enabled()
        error('Cannot link with libudev')
      else
        warning('Cannot link with libudev, disabling')
        libudev = not_found
      endif
    endif
  endif
endif

It can be done in meson.build also for dependencies that are still
detected in configure, and we can place it in a loop if we want to do
many tests like these:

if enable_static
  dependencies = {'libudev': 'mpath', ...}
  skeleton = 'int main(void) { return 0; }'
  foreach var, option: dependencies
    if not cc.links(skeleton, dependencies: get_variable(var))
      if get_option(option).enabled()
          error('Cannot link with @0@'.format(var))
        else
          warning('Cannot link with @0@, disabling'.format(skeleton))
          set_variable(var, not_found)
        endif
      endif
    endif
  endforeach
endif

This way we don't have 500 lines of this kind of test.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/10/20 14:35, Peter Maydell wrote:
> >
> > It would be better to do the "see if a static library is present"
> > test. This isn't too hard to do in configure (compare that
> > six line fix to the detection of libgio). Hopefully it is
> > not too hard to do in meson ?
>
> Yes, something like:
>
> if enable_static
>   skeleton = 'int main(void) { return 0; }'
>   if not cc.links(skeleton, dependencies: libudev)
>     if get_option('mpath').enabled()
>         error('Cannot link with libudev')
>       else
>         warning('Cannot link with libudev, disabling')
>         libudev = not_found
>       endif
>     endif
>   endif
> endif

This duplicates the information that the thing that depends
on libudev is mpath. Can we put this in a wrapper around
dependency() so that we could just say something like
  libudev = compile_checked_dependency('libudev',
                       required: get_option('mpath').enabled(),
                       static: enable_static)

for those dependencies that want to do the "does this compile"
check ?

thanks
-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 15:09, Peter Maydell wrote:
> This duplicates the information that the thing that depends
> on libudev is mpath. Can we put this in a wrapper around
> dependency() so that we could just say something like
>   libudev = compile_checked_dependency('libudev',
>                        required: get_option('mpath').enabled(),
>                        static: enable_static)
> 
> for those dependencies that want to do the "does this compile"
> check ?

No, there's no functions at all.  You can of course put the detection and
test in a single loop:

  dependencies = {}
  ...
  if targetos == 'linux' and (have_system or have_tools)
    dependencies += {'libudev': 'mpath'}
  endif
  ...
  skeleton = 'int main(void) { return 0; }'
  foreach var, option: dependencies
    dep = dependency(var,
                     required: get_option(option).enabled(),
                     static: enable_static)
    if dep.found() and enable_static and not cc.links(skeleton, dependencies: get_variable(var))
      if get_option(option).enabled()
          error('Cannot link with @0@'.format(var))
        else
          warning('Cannot link with @0@, disabling'.format(skeleton))
          set_variable(var, not_found)
        endif
      endif
    endif
  endforeach

Doing this check for all libraries is certainly a good idea.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 15:08, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/10/20 15:09, Peter Maydell wrote:
> > This duplicates the information that the thing that depends
> > on libudev is mpath. Can we put this in a wrapper around
> > dependency() so that we could just say something like
> >   libudev = compile_checked_dependency('libudev',
> >                        required: get_option('mpath').enabled(),
> >                        static: enable_static)
> >
> > for those dependencies that want to do the "does this compile"
> > check ?
>
> No, there's no functions at all.  You can of course put the detection and
> test in a single loop:
>
>   dependencies = {}
>   ...
>   if targetos == 'linux' and (have_system or have_tools)
>     dependencies += {'libudev': 'mpath'}
>   endif
>   ...
>   skeleton = 'int main(void) { return 0; }'
>   foreach var, option: dependencies
>     dep = dependency(var,
>                      required: get_option(option).enabled(),
>                      static: enable_static)
>     if dep.found() and enable_static and not cc.links(skeleton, dependencies: get_variable(var))
>       if get_option(option).enabled()
>           error('Cannot link with @0@'.format(var))
>         else
>           warning('Cannot link with @0@, disabling'.format(skeleton))
>           set_variable(var, not_found)
>         endif
>       endif
>     endif
>   endforeach

That is a lot uglier. I'm really getting fed up with Meson's
persistent "no, you can't do that" attitude :-(

thanks
-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 02/10/20 16:18, Peter Maydell wrote:
>> No, there's no functions at all.  You can of course put the detection and
>> test in a single loop:
>>
>>   dependencies = {}
>>   ...
>>   if targetos == 'linux' and (have_system or have_tools)
>>     dependencies += {'libudev': 'mpath'}
>>   endif
>>   ...
>>   skeleton = 'int main(void) { return 0; }'
>>   foreach var, option: dependencies
>>     dep = dependency(var,
>>                      required: get_option(option).enabled(),
>>                      static: enable_static)
>>     if dep.found() and enable_static and not cc.links(skeleton, dependencies: get_variable(var))
>>       if get_option(option).enabled()
>>           error('Cannot link with @0@'.format(var))
>>         else
>>           warning('Cannot link with @0@, disabling'.format(skeleton))
>>           set_variable(var, not_found)
>>         endif
>>       endif
>>     endif
>>   endforeach
> That is a lot uglier.

The code above is ugly but it is also botched; it should be more like:

  dependencies = {}
  ...
  if targetos == 'linux' and (have_system or have_tools)
    dependencies += {'libudev': get_option('mpath')}
  else
    libudev = not_found
  endif
  ...

  skeleton = 'int main(void) { return 0; }'
  foreach var, opt: dependencies
    dep = dependency(var,
                     required: opt.enabled(),
                     static: enable_static)
    if dep.found() and enable_static and not cc.links(skeleton, dependencies: dep)
      # Meson should have already warned about the lack of a static library
      if opt.enabled()
        error('Cannot link with @0@'.format(var))
      else
      dep = not_found
    endif
    set_variable(var, dep)
  endforeach

which both shorter and more readable.

Or is it loops vs. functions that you find ugly?

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

On Fri, 2 Oct 2020 at 16:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Or is it loops vs. functions that you find ugly?

Lack of functions. Being able to abstract out what you're
doing into something with a comprehensible name is a
pretty basic software engineering requirement. Yes, you
*can* rearrange everything so that you ensure that
all the code that wants to do something sets up
some variables and then you loop over them, but
then you're contorting things out of the most readable
way to express them to work around limitations of the
build tool.

thanks
-- PMM

Re: [PATCH] meson.build: Don't look for libudev for static builds

[罗勇刚(Yonggang Luo)]

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

On Fri, Oct 2, 2020 at 9:11 PM Peter Maydell <peter.maydell@linaro.org>
wrote:
>
> On Fri, 2 Oct 2020 at 14:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 02/10/20 14:35, Peter Maydell wrote:
> > >
> > > It would be better to do the "see if a static library is present"
> > > test. This isn't too hard to do in configure (compare that
> > > six line fix to the detection of libgio). Hopefully it is
> > > not too hard to do in meson ?
> >
> > Yes, something like:
> >
> > if enable_static
> >   skeleton = 'int main(void) { return 0; }'
> >   if not cc.links(skeleton, dependencies: libudev)
> >     if get_option('mpath').enabled()
> >         error('Cannot link with libudev')
> >       else
> >         warning('Cannot link with libudev, disabling')
> >         libudev = not_found
> >       endif
> >     endif
> >   endif
> > endif
>
> This duplicates the information that the thing that depends
> on libudev is mpath. Can we put this in a wrapper around
> dependency() so that we could just say something like
>   libudev = compile_checked_dependency('libudev',
>                        required: get_option('mpath').enabled(),
>                        static: enable_static)
>
Hi Bonzini,
This looks like a frequently used function, can we upstrem to meson?


> for those dependencies that want to do the "does this compile"
> check ?
>
> thanks
> -- PMM
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 2084 bytes --]

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 03/10/20 09:24, 罗勇刚(Yonggang Luo) wrote:
> 
> 
> On Fri, Oct 2, 2020 at 9:11 PM Peter Maydell <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> wrote:
>>
>> On Fri, 2 Oct 2020 at 14:05, Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
>> >
>> > On 02/10/20 14:35, Peter Maydell wrote:
>> > >
>> > > It would be better to do the "see if a static library is present"
>> > > test. This isn't too hard to do in configure (compare that
>> > > six line fix to the detection of libgio). Hopefully it is
>> > > not too hard to do in meson ?
>> >
>> > Yes, something like:
>> >
>> > if enable_static
>> >   skeleton = 'int main(void) { return 0; }'
>> >   if not cc.links(skeleton, dependencies: libudev)
>> >     if get_option('mpath').enabled()
>> >         error('Cannot link with libudev')
>> >       else
>> >         warning('Cannot link with libudev, disabling')
>> >         libudev = not_found
>> >       endif
>> >     endif
>> >   endif
>> > endif
>>
>> This duplicates the information that the thing that depends
>> on libudev is mpath. Can we put this in a wrapper around
>> dependency() so that we could just say something like
>>   libudev = compile_checked_dependency('libudev',
>>                        required: get_option('mpath').enabled(),
>>                        static: enable_static)
>>
> Hi Bonzini,
> This looks like a frequently used function, can we upstrem to meson?

Yes, I think adding a "links" argument to dependency (similar to
find_library's has_headers argument) makes sense.  That would be written

    dependency('libudev',
               required: get_option('mpath').enabled(),
               static: enable_static,
               links: skeleton)

But anyway that shouldn't be a blocker for more improvements to qemu's
meson.build.  Now that we have 5-10 dependencies converted we have a
clearer idea of how to abstract the tests.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[罗勇刚(Yonggang Luo)]

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

On Sat, Oct 3, 2020 at 3:50 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 03/10/20 09:24, 罗勇刚(Yonggang Luo) wrote:
> >
> >
> > On Fri, Oct 2, 2020 at 9:11 PM Peter Maydell <peter.maydell@linaro.org
> > <mailto:peter.maydell@linaro.org>> wrote:
> >>
> >> On Fri, 2 Oct 2020 at 14:05, Paolo Bonzini <pbonzini@redhat.com
> > <mailto:pbonzini@redhat.com>> wrote:
> >> >
> >> > On 02/10/20 14:35, Peter Maydell wrote:
> >> > >
> >> > > It would be better to do the "see if a static library is present"
> >> > > test. This isn't too hard to do in configure (compare that
> >> > > six line fix to the detection of libgio). Hopefully it is
> >> > > not too hard to do in meson ?
> >> >
> >> > Yes, something like:
> >> >
> >> > if enable_static
> >> >   skeleton = 'int main(void) { return 0; }'
> >> >   if not cc.links(skeleton, dependencies: libudev)
> >> >     if get_option('mpath').enabled()
> >> >         error('Cannot link with libudev')
> >> >       else
> >> >         warning('Cannot link with libudev, disabling')
> >> >         libudev = not_found
> >> >       endif
> >> >     endif
> >> >   endif
> >> > endif
> >>
> >> This duplicates the information that the thing that depends
> >> on libudev is mpath. Can we put this in a wrapper around
> >> dependency() so that we could just say something like
> >>   libudev = compile_checked_dependency('libudev',
> >>                        required: get_option('mpath').enabled(),
> >>                        static: enable_static)
> >>
> > Hi Bonzini,
> > This looks like a frequently used function, can we upstrem to meson?
>
> Yes, I think adding a "links" argument to dependency (similar to
> find_library's has_headers argument) makes sense.  That would be written
>
>     dependency('libudev',
>                required: get_option('mpath').enabled(),
>                static: enable_static,
>                links: skeleton)
make sense, may also need extra cflags and link flags.
>
> But anyway that shouldn't be a blocker for more improvements to qemu's
> meson.build.  Now that we have 5-10 dependencies converted we have a
> clearer idea of how to abstract the tests.
>
> Paolo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 3351 bytes --]

Re: [PATCH] meson.build: Don't look for libudev for static builds

[罗勇刚(Yonggang Luo)]

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

On Sat, Oct 3, 2020 at 3:50 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 03/10/20 09:24, 罗勇刚(Yonggang Luo) wrote:
> >
> >
> > On Fri, Oct 2, 2020 at 9:11 PM Peter Maydell <peter.maydell@linaro.org
> > <mailto:peter.maydell@linaro.org>> wrote:
> >>
> >> On Fri, 2 Oct 2020 at 14:05, Paolo Bonzini <pbonzini@redhat.com
> > <mailto:pbonzini@redhat.com>> wrote:
> >> >
> >> > On 02/10/20 14:35, Peter Maydell wrote:
> >> > >
> >> > > It would be better to do the "see if a static library is present"
> >> > > test. This isn't too hard to do in configure (compare that
> >> > > six line fix to the detection of libgio). Hopefully it is
> >> > > not too hard to do in meson ?
> >> >
> >> > Yes, something like:
> >> >
> >> > if enable_static
> >> >   skeleton = 'int main(void) { return 0; }'
> >> >   if not cc.links(skeleton, dependencies: libudev)
> >> >     if get_option('mpath').enabled()
> >> >         error('Cannot link with libudev')
> >> >       else
> >> >         warning('Cannot link with libudev, disabling')
> >> >         libudev = not_found
> >> >       endif
> >> >     endif
> >> >   endif
> >> > endif
> >>
> >> This duplicates the information that the thing that depends
> >> on libudev is mpath. Can we put this in a wrapper around
> >> dependency() so that we could just say something like
> >>   libudev = compile_checked_dependency('libudev',
> >>                        required: get_option('mpath').enabled(),
> >>                        static: enable_static)
> >>
> > Hi Bonzini,
> > This looks like a frequently used function, can we upstrem to meson?
>
> Yes, I think adding a "links" argument to dependency (similar to
> find_library's has_headers argument) makes sense.  That would be written
>
>     dependency('libudev',
>                required: get_option('mpath').enabled(),
>                static: enable_static,
>                links: skeleton)
>
For some meson script like this:
curses = not_found
if iconv.found() and not get_option('curses').disabled()
  curses_libname_list = ['ncursesw', 'ncurses', 'cursesw', 'pdcurses']
  curses_test = '''
    #include <locale.h>
    #include <curses.h>
    #include <wchar.h>
    int main(void) {
      wchar_t wch = L'w';
      setlocale(LC_ALL, "");
      resize_term(0, 0);
      addwstr(L"wide chars\n");
      addnwstr(&wch, 1);
      add_wch(WACS_DEGREE);
      return 0;
    }'''
  foreach curses_libname : curses_libname_list
      libcurses = dependency(curses_libname,
                             required: false,
                             method: 'pkg-config',
                             static: enable_static)

      if not libcurses.found()
        dirs = ['/usr/include/ncursesw']
        if targetos == 'windows'
          dirs = []
        endif
        libcurses = cc.find_library(curses_libname,
                                    required: false,
                                    dirs: dirs,
                                    static: enable_static)
      endif
      if libcurses.found()
        if cc.links(curses_test, dependencies: [libcurses])
          curses = declare_dependency(compile_args: '-DNCURSES_WIDECHAR',
dependencies: [libcurses])
          break
        endif
      endif
  endforeach
endif

We also need to define extra  compile_args  '-DNCURSES_WIDECHAR' as the
part of dependencies.

> But anyway that shouldn't be a blocker for more improvements to qemu's
> meson.build.  Now that we have 5-10 dependencies converted we have a
> clearer idea of how to abstract the tests.
>
> Paolo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 5243 bytes --]

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 03/10/20 10:28, 罗勇刚(Yonggang Luo) wrote:
>> Yes, I think adding a "links" argument to dependency (similar to
>> find_library's has_headers argument) makes sense.  That would be written
>>
>>     dependency('libudev',
>>                required: get_option('mpath').enabled(),
>>                static: enable_static,
>>                links: skeleton)
> make sense, may also need extra cflags and link flags.

They would be provided by the dependency, in general.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[Paolo Bonzini]

On 03/10/20 10:29, 罗勇刚(Yonggang Luo) wrote:
> For some meson script like this:
> curses = not_found
> if iconv.found() and not get_option('curses').disabled()
>   curses_libname_list = ['ncursesw', 'ncurses', 'cursesw', 'pdcurses']
>   curses_test = '''
>     #include <locale.h>
>     #include <curses.h>
>     #include <wchar.h>
>     int main(void) {
>       wchar_t wch = L'w';
>       setlocale(LC_ALL, "");
>       resize_term(0, 0);
>       addwstr(L"wide chars\n");
>       addnwstr(&wch, 1);
>       add_wch(WACS_DEGREE);
>       return 0;
>     }'''
>   foreach curses_libname : curses_libname_list
>       libcurses = dependency(curses_libname,
>                              required: false,
>                              method: 'pkg-config',
>                              static: enable_static)
> 
>       if not libcurses.found()
>         dirs = ['/usr/include/ncursesw']
>         if targetos == 'windows'
>           dirs = []
>         endif
>         libcurses = cc.find_library(curses_libname,
>                                     required: false,
>                                     dirs: dirs,
>                                     static: enable_static)
>       endif
>       if libcurses.found()
>         if cc.links(curses_test, dependencies: [libcurses])
>           curses = declare_dependency(compile_args:
> '-DNCURSES_WIDECHAR', dependencies: [libcurses])
>           break
>         endif
>       endif
>   endforeach
> endif
> 
> We also need to define extra  compile_args  '-DNCURSES_WIDECHAR' as the
> part of dependencies.

You can do that with #define before including <curses.h>.

Paolo

Re: [PATCH] meson.build: Don't look for libudev for static builds

[罗勇刚(Yonggang Luo)]

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

On Sat, Oct 3, 2020 at 4:43 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 03/10/20 10:29, 罗勇刚(Yonggang Luo) wrote:
> > For some meson script like this:
> > curses = not_found
> > if iconv.found() and not get_option('curses').disabled()
> >   curses_libname_list = ['ncursesw', 'ncurses', 'cursesw', 'pdcurses']
> >   curses_test = '''
> >     #include <locale.h>
> >     #include <curses.h>
> >     #include <wchar.h>
> >     int main(void) {
> >       wchar_t wch = L'w';
> >       setlocale(LC_ALL, "");
> >       resize_term(0, 0);
> >       addwstr(L"wide chars\n");
> >       addnwstr(&wch, 1);
> >       add_wch(WACS_DEGREE);
> >       return 0;
> >     }'''
> >   foreach curses_libname : curses_libname_list
> >       libcurses = dependency(curses_libname,
> >                              required: false,
> >                              method: 'pkg-config',
> >                              static: enable_static)
> >
> >       if not libcurses.found()
> >         dirs = ['/usr/include/ncursesw']
> >         if targetos == 'windows'
> >           dirs = []
> >         endif
> >         libcurses = cc.find_library(curses_libname,
> >                                     required: false,
> >                                     dirs: dirs,
> >                                     static: enable_static)
> >       endif
> >       if libcurses.found()
> >         if cc.links(curses_test, dependencies: [libcurses])
> >           curses = declare_dependency(compile_args:
> > '-DNCURSES_WIDECHAR', dependencies: [libcurses])
> >           break
> >         endif
> >       endif
> >   endforeach
> > endif
> >
> > We also need to define extra  compile_args  '-DNCURSES_WIDECHAR' as the
> > part of dependencies.
>
> You can do that with #define before including <curses.h>.
Yeap, but the  -DNCURSES_WIDECHAR not only used for testing compile of
#include<curses.h> but also
as a dependencies of qemu.
>
> Paolo
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 2944 bytes --]

[PATCH] meson.build: Don't look for libudev for static builds

[Peter Maydell]

commit f01496a314d916 moved the logic for detecting libudev from
configure to meson.build, but in the process it dropped the condition
that meant we only ask pkg-config about libudev for a non-static
build.

This breaks static builds of the system emulators on at least Ubuntu
18.04.4, because on that host there is no static libudev but
pkg-config still claims it exists.

Reinstate the logic that we had in the configure check.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
We could certainly do something cleverer here, but basic "convert
from configure to meson" should in general not also be changing the
detection logic IMHO.  We can make the logic smarter as a follow-on
patch if desired.
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 3161c1f037a..07da66e1d81 100644
--- a/meson.build
+++ b/meson.build
@@ -271,7 +271,7 @@ if 'CONFIG_CURL' in config_host
                             link_args: config_host['CURL_LIBS'].split())
 endif
 libudev = not_found
-if targetos == 'linux' and (have_system or have_tools)
+if targetos == 'linux' and (have_system or have_tools) and not enable_static
   libudev = dependency('libudev',
                        required: get_option('mpath').enabled(),
                        static: enable_static)
-- 
2.20.1