[Qemu-devel] [PATCH] configure: try pkg-config ncurses first

[Qemu-devel] [PATCH] configure: try pkg-config ncurses first

[Ed Maste]

When probing for ncurses, try pkg-config first rather than after
explicit -lncurses and -lcurses.  This fixes static linking in the case
that ncurses has additional dependencies, such as -ltinfo (as on FreeBSD).

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index cfdb564..7c99ef9 100755
--- a/configure
+++ b/configure
@@ -2157,7 +2157,7 @@ fi
 if test "$mingw32" = "yes" ; then
     curses_list="-lpdcurses"
 else
-    curses_list="-lncurses:-lcurses:$($pkg_config --libs ncurses 2>/dev/null)"
+    curses_list="$($pkg_config --libs ncurses 2>/dev/null):-lncurses:-lcurses"
 fi
 
 if test "$curses" != "no" ; then
-- 
1.7.11.5

Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: try pkg-config ncurses first

[Michael Tokarev]

25.05.2013 00:07, Ed Maste wrote:
> When probing for ncurses, try pkg-config first rather than after
> explicit -lncurses and -lcurses.  This fixes static linking in the case
> that ncurses has additional dependencies, such as -ltinfo (as on FreeBSD).

This is not a FreeBSD-specific thing, this is the way how current
ncurses works -- they separated a bunch of functions into a new
library, libtinfo.

But this is interesting.

I'm not sure I agree with this approach.  When we're building using
shared library, libncurses.so already links with libtinfo.so, so we
don't need to link executable itself with libtinfo.so, since executable
itself uses none of its functions.

On the other hand, the current logic appears to be fine, -- we first
link with just -lncurses, and if that fails, we also try pkg-config --libs --
because, maybe, we're linking statically and in that case, additional
libs from pkg-config may help.

>From yet another point of view, we may use --as-needed linker flag
and just ignore all the above.

Here, it is interesting to note that pkg-config does not actually do
the right thing in this case.  Because practically, it should have
one extra flag, something like --static-libs (or --libs --static),
and it should actually be different from plain --libs.

Anyway, I don't see a reason to apply this as it is.

Thanks,

/mjt

> 
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
>  configure | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index cfdb564..7c99ef9 100755
> --- a/configure
> +++ b/configure
> @@ -2157,7 +2157,7 @@ fi
>  if test "$mingw32" = "yes" ; then
>      curses_list="-lpdcurses"
>  else
> -    curses_list="-lncurses:-lcurses:$($pkg_config --libs ncurses 2>/dev/null)"
> +    curses_list="$($pkg_config --libs ncurses 2>/dev/null):-lncurses:-lcurses"
>  fi
>  
>  if test "$curses" != "no" ; then
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: try pkg-config ncurses first

[Ed Maste]

On 25 May 2013 00:25, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 25.05.2013 00:07, Ed Maste wrote:
>> When probing for ncurses, try pkg-config first rather than after
>> explicit -lncurses and -lcurses.  This fixes static linking in the case
>> that ncurses has additional dependencies, such as -ltinfo (as on FreeBSD).
>
> This is not a FreeBSD-specific thing, this is the way how current
> ncurses works -- they separated a bunch of functions into a new
> library, libtinfo.

Right, I don't mean to apply it was FreeBSD-specific.

Presumably QEMU builds with --static on at least some Linux distros,
so I assumed they must not have the external libtinfo dep for static
linking (because they build libncurses.a with it built-in, say).

> But this is interesting.
>
> I'm not sure I agree with this approach.  When we're building using
> shared library, libncurses.so already links with libtinfo.so, so we
> don't need to link executable itself with libtinfo.so, since executable
> itself uses none of its functions.

Right, and pkg-config doesn't include -ltinfo when using dynamic linking.

> On the other hand, the current logic appears to be fine, -- we first
> link with just -lncurses, and if that fails, we also try pkg-config --libs --
> because, maybe, we're linking statically and in that case, additional
> libs from pkg-config may help.

Except that the test snippet in configure will successfully link
statically without -ltinfo, so configure doesn't make it to the
pkg-config test.

> From yet another point of view, we may use --as-needed linker flag
> and just ignore all the above.
>
> Here, it is interesting to note that pkg-config does not actually do
> the right thing in this case.  Because practically, it should have
> one extra flag, something like --static-libs (or --libs --static),
> and it should actually be different from plain --libs.

In fact, it does:

feynman% pkg-config --libs ncurses
-L/usr/local/lib -lncurses
feynman% pkg-config --static --libs ncurses
-L/usr/local/lib -lncurses -ltinfo

and the configure script adds --static to QEMU_PKG_CONFIG_FLAGS when necessary.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: try pkg-config ncurses first

[Andreas Färber]

Am 25.05.2013 06:25, schrieb Michael Tokarev:
> 25.05.2013 00:07, Ed Maste wrote:
>> When probing for ncurses, try pkg-config first rather than after
>> explicit -lncurses and -lcurses.  This fixes static linking in the case
>> that ncurses has additional dependencies, such as -ltinfo (as on FreeBSD).
> 
> This is not a FreeBSD-specific thing, this is the way how current
> ncurses works -- they separated a bunch of functions into a new
> library, libtinfo.
> 
> But this is interesting.
> 
> I'm not sure I agree with this approach.  When we're building using
> shared library, libncurses.so already links with libtinfo.so, so we
> don't need to link executable itself with libtinfo.so, since executable
> itself uses none of its functions.
> 
> On the other hand, the current logic appears to be fine, -- we first
> link with just -lncurses, and if that fails, we also try pkg-config --libs --
> because, maybe, we're linking statically and in that case, additional
> libs from pkg-config may help.

I think the submitter wanted to assure that pkg-config --libs ncurses is
tried *before* falling back to -lcurses? (Rather than caring about the
order of -lncurses vs. pkg-config --libs ncurses.)

> From yet another point of view, we may use --as-needed linker flag
> and just ignore all the above.
> 
> Here, it is interesting to note that pkg-config does not actually do
> the right thing in this case.  Because practically, it should have
> one extra flag, something like --static-libs (or --libs --static),
> and it should actually be different from plain --libs.

$pkg_config should already contain --static for static builds, so I see
nothing wrong with this patch,

Reviewed-by: Andreas Färber <afaerber@suse.de>

but maybe I'm missing something?

One thing to note is that in an unfortunate layering violation unicore32
uses curses, so static libs and their potential need to link in
additional dependencies may be more than just a theoretical concern.

Regards,
Andreas

> Anyway, I don't see a reason to apply this as it is.
> 
> Thanks,
> 
> /mjt
> 
>>
>> Signed-off-by: Ed Maste <emaste@freebsd.org>
>> ---
>>  configure | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index cfdb564..7c99ef9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -2157,7 +2157,7 @@ fi
>>  if test "$mingw32" = "yes" ; then
>>      curses_list="-lpdcurses"
>>  else
>> -    curses_list="-lncurses:-lcurses:$($pkg_config --libs ncurses 2>/dev/null)"
>> +    curses_list="$($pkg_config --libs ncurses 2>/dev/null):-lncurses:-lcurses"
>>  fi
>>  
>>  if test "$curses" != "no" ; then
>>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: try pkg-config ncurses first

[Michael Tokarev]

25.05.2013 15:38, Andreas Färber wrote:
> Am 25.05.2013 06:25, schrieb Michael Tokarev:
[]
>> Here, it is interesting to note that pkg-config does not actually do
>> the right thing in this case.  Because practically, it should have
>> one extra flag, something like --static-libs (or --libs --static),
>> and it should actually be different from plain --libs.
> 
> $pkg_config should already contain --static for static builds, so I see
> nothing wrong with this patch,

Hmm. I didn't know.  Okay.

On my system pkg-config does the same thing for ncurses, be it
with --static or without -- in both cases it prints `-lncurses -ltinfo',
so after this patch, qemu executable itself will be linked with a library
it does not use directly.  Definitely not a big deal, because it is used
by -lncurses anyway (indirectly) and so both libs are actually required.

When qemu is built on debian, we include --as-needed flag for the linker
as well, in order to get rid of all those extra but unused libs, so it
does not matter for us anyway.

And since apparenlty this is how ncurses is supposed to work/used, I
guess nothing is wrong with that at all ;)

> Reviewed-by: Andreas Färber <afaerber@suse.de>
> 
> but maybe I'm missing something?

I don't think so.  The only by concern was that the new (which is
apparently correct, and if not, we should complain to ncurses not
qemu) is the extra unnecessary library which is being linked to
qemu directly when doing non-static build.

> One thing to note is that in an unfortunate layering violation unicore32
> uses curses, so static libs and their potential need to link in
> additional dependencies may be more than just a theoretical concern.

How unicore32 is different from everything else?

It definitely is not a theorerical concern, -- for example, debian
build static binaries for all user targets, to simplify usage of
"foreign chroots", -- you install a foreign system in a chroot,
drop the right qemu-$arch-static binary into its usr/bin, and
just chroot to it.  So static building is needed and used, but with
that, configure finds right libs anyway, because it tries -lncurses
which fails without -ltinfo, and next it tries stuff from pkg-config
which works.

With this --static flag to pkg-config, things become instantly
correct too.

I'll apply it on top of my configure changes.

Thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] configure: try pkg-config ncurses first

[Michael Tokarev]

25.05.2013 00:07, Ed Maste wrote:
> When probing for ncurses, try pkg-config first rather than after
> explicit -lncurses and -lcurses.  This fixes static linking in the case
> that ncurses has additional dependencies, such as -ltinfo (as on FreeBSD).

Thanks, applied to the trivial patches queue, on top of my tiny configure change.

/mjt