| Message ID | 1369426020-39060-1-git-send-email-emaste@freebsd.org |
|---|---|
| State | New |
| Headers | show |
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 >
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.
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 >>
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
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
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
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(-)