[libvirt] [PATCH 1/4] Avoid searching for windres when not building for cygwin or mingw.
Just checking for a windres tool might hit even on Linux systems when building for Linux (e.g.: when using Gentoo and having built binutils with multitarget support), and will then fail to link properly at the end of the build. Check the host string before deciding whether to look for windres or not. --- configure.ac | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 117cb20..5b1eb5f 100644 --- a/configure.ac +++ b/configure.ac @@ -1682,8 +1682,13 @@ AC_SUBST([CYGWIN_EXTRA_PYTHON_LIBADD]) AC_SUBST([MINGW_EXTRA_LDFLAGS]) dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""]) -- 1.7.0
Without this change, gcry_control() and gcry_check_version() will be undefined in libvirt.so (src/libvirt.c:331) when building drivers as modules rather than built-in. --- src/Makefile.am | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 2051e5d..46acb7d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -336,6 +336,7 @@ libvirt_driver_la_SOURCES = $(DRIVER_SOURCES) libvirt_driver_la_CFLAGS = $(NUMACTL_CFLAGS) \ -I@top_srcdir@/src/conf libvirt_driver_la_LDFLAGS = $(NUMACTL_LIBS) +libvirt_driver_la_LIBADD = $(GNUTLS_LIBS) USED_SYM_FILES = libvirt_private.syms -- 1.7.0
On Wed, Feb 24, 2010 at 04:04:31PM +0100, Diego Elio Pettenò wrote:
Without this change, gcry_control() and gcry_check_version() will be undefined in libvirt.so (src/libvirt.c:331) when building drivers as modules rather than built-in. --- src/Makefile.am | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 2051e5d..46acb7d 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -336,6 +336,7 @@ libvirt_driver_la_SOURCES = $(DRIVER_SOURCES) libvirt_driver_la_CFLAGS = $(NUMACTL_CFLAGS) \ -I@top_srcdir@/src/conf libvirt_driver_la_LDFLAGS = $(NUMACTL_LIBS) +libvirt_driver_la_LIBADD = $(GNUTLS_LIBS)
USED_SYM_FILES = libvirt_private.syms
ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
Since on not all systems dlopen() is in libdl, add a very simple macro that allows to search for it and sets a DLOPEN_LIBS variable with its value to be used. Use the macro instead of hardcoding -ldl for the drivers-as-modules case. --- configure.ac | 4 ++-- m4/libdl.m4 | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 m4/libdl.m4 diff --git a/configure.ac b/configure.ac index 5b1eb5f..4a848cd 100644 --- a/configure.ac +++ b/configure.ac @@ -1705,7 +1705,7 @@ if test "x$with_driver_modules" = "xyes" ; then old_libs="$LIBS" fail=0 AC_CHECK_HEADER([dlfcn.h],[],[fail=1]) - AC_CHECK_LIB([dl], [dlopen],[],[fail=1]) + AFX_LIB_DLOPEN([], [fail=1]) test $fail = 1 && AC_MSG_ERROR([You must have dlfcn.h / dlopen() support to build driver modules]) @@ -1714,7 +1714,7 @@ if test "x$with_driver_modules" = "xyes" ; then fi if test "$with_driver_modules" = "yes"; then DRIVER_MODULES_CFLAGS="-export-dynamic" - DRIVER_MODULES_LIBS="-ldl" + DRIVER_MODULES_LIBS="$DLOPEN_LIBS" AC_DEFINE_UNQUOTED([WITH_DRIVER_MODULES], 1, [whether to build drivers as modules]) fi AM_CONDITIONAL([WITH_DRIVER_MODULES], [test "$with_driver_modules" != "no"]) diff --git a/m4/libdl.m4 b/m4/libdl.m4 new file mode 100644 index 0000000..84b6a2c --- /dev/null +++ b/m4/libdl.m4 @@ -0,0 +1,44 @@ +dnl Copyright (c) 2010 Diego Elio Pettenò <flameeyes@gmail.com> +dnl +dnl Permission is hereby granted, free of charge, to any person +dnl obtaining a copy of this software and associated documentation +dnl files (the "Software"), to deal in the Software without +dnl restriction, including without limitation the rights to use, +dnl copy, modify, merge, publish, distribute, sublicense, and/or sell +dnl copies of the Software, and to permit persons to whom the +dnl Software is furnished to do so, subject to the following +dnl conditions: +dnl +dnl The above copyright notice and this permission notice shall be +dnl included in all copies or substantial portions of the Software. +dnl +dnl THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +dnl EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +dnl OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +dnl NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +dnl HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +dnl WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +dnl FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +dnl OTHER DEALINGS IN THE SOFTWARE. + +dnl AFX to get a unique namespace before it is submitted somewhere +dnl (like autoconf-archive). +AC_DEFUN([AFX_LIB_DLOPEN], [ + DLOPEN_LIBS= + +m4_pushdef([action_if_not_found], + [m4_default([$2], + [AC_MSG_ERROR([Unable to find the dlopen() function])] + ) + ]) + + old_libs=$LIBS + AC_SEARCH_LIBS([dlopen], [dl], + [DLOPEN_LIBS=${LIBS#${old_libs}} + $1], action_if_not_found) + LIBS=$old_libs + +m4_popdef([action_if_not_found]) + + AC_SUBST([DLOPEN_LIBS]) +]) -- 1.7.0
According to Diego Elio Pettenò on 2/24/2010 8:04 AM:
+dnl AFX to get a unique namespace before it is submitted somewhere +dnl (like autoconf-archive). +AC_DEFUN([AFX_LIB_DLOPEN], [
Just curious: Why the choice of AFX_, and not something like LV_ for libvirt?
+ DLOPEN_LIBS= + +m4_pushdef([action_if_not_found], + [m4_default([$2], + [AC_MSG_ERROR([Unable to find the dlopen() function])] + ) + ]) + + old_libs=$LIBS + AC_SEARCH_LIBS([dlopen], [dl], + [DLOPEN_LIBS=${LIBS#${old_libs}}
The shell expansion ${a#b} is not portable to Solaris /bin/sh. Is that a problem for libvirt, or are we Linux-centric enough to not care?
+ $1], action_if_not_found)
Underquoted; if the AC_MSG_ERROR were ever changed to contain a comma, then this would be passing an unintentional extra argument to AC_SEARCH_LIBS. It should work to pass [action_if_not_found] rather than action_if_not_found, or even avoid the temporary macro altogether and inline the m4_default directly as the argument to AC_SEARCH_LIBS. Is this something that would be worth wrapping in an AC_CACHE_CHECK, to avoid repeating the check if config.cache exists? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Il giorno mer, 24/02/2010 alle 08.21 -0700, Eric Blake ha scritto:
Just curious: Why the choice of AFX_, and not something like LV_ for libvirt?
Because I plan to finish this up and send it down to autoconf-archive asap as it's not the first time I have to get something like this ;)
The shell expansion ${a#b} is not portable to Solaris /bin/sh. Is that a problem for libvirt, or are we Linux-centric enough to not care?
That's definitely a problem I didn't think about, I was expecting POSIX shell to support that, but I'll look for another way.
Underquoted; if the AC_MSG_ERROR were ever changed to contain a comma, then this would be passing an unintentional extra argument to AC_SEARCH_LIBS.
I have to double check that one but I think that if I were to quote that it wouldn't expand properly.
Is this something that would be worth wrapping in an AC_CACHE_CHECK, to avoid repeating the check if config.cache exists?
The inner check is already cached, so I don't think there is much need for adding one further cache… But this actually made me think that we might as well just use AC_SEARCH_LIB and rely on the value stored in the cache to begin with so I'll send a different version in a moment. -- Diego Elio Pettenò — “Flameeyes” http://blog.flameeyes.eu/ If you found a .asc file in this mail and know not what it is, it's a GnuPG digital signature: http://www.gnupg.org/
According to Diego Elio “Flameeyes” Pettenò on 2/24/2010 8:27 AM:
The shell expansion ${a#b} is not portable to Solaris /bin/sh. Is that a problem for libvirt, or are we Linux-centric enough to not care?
That's definitely a problem I didn't think about, I was expecting POSIX shell to support that, but I'll look for another way.
POSIX, yes. But Solaris /bin/sh is not POSIX.
Underquoted; if the AC_MSG_ERROR were ever changed to contain a comma, then this would be passing an unintentional extra argument to AC_SEARCH_LIBS.
I have to double check that one but I think that if I were to quote that it wouldn't expand properly.
Maybe the alternative is to use m4_default_quoted (oops, not portable to older autoconf), or m4_default([$2], [[AC_MSG_ERROR([])]]) in the definition of action_if_not_found. The problem I'm trying to avoid is that you should be sure that AC_SEARCH_LIBS sees [AC_MSG_ERROR([])], and not the underquoted AC_MSG_ERROR([]), so that the contents of the error message can safely be changed to have m4 metacharacters without any impact.
Is this something that would be worth wrapping in an AC_CACHE_CHECK, to avoid repeating the check if config.cache exists?
The inner check is already cached, so I don't think there is much need for adding one further cache…
Good point. All of the heavy lifting is already in AC_SEARCH_LIBS, so adding another cache layer doesn't buy anything.
But this actually made me think that we might as well just use AC_SEARCH_LIB and rely on the value stored in the cache to begin with so I'll send a different version in a moment.
Which may render all of the above comments obsolete, depending on what you come up with. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 02/24/2010 04:21 PM, Eric Blake wrote:
+ AC_SEARCH_LIBS([dlopen], [dl], + [DLOPEN_LIBS=${LIBS#${old_libs}}
The shell expansion ${a#b} is not portable to Solaris /bin/sh. Is that a problem for libvirt, or are we Linux-centric enough to not care?
It should definitely configure right on Solaris, however it's simpler and better to use a case statement on the cache variable instead. Paolo
On Wed, Feb 24, 2010 at 04:04:32PM +0100, Diego Elio Pettenò wrote:
Since on not all systems dlopen() is in libdl, add a very simple macro that allows to search for it and sets a DLOPEN_LIBS variable with its value to be used.
The autoconf change looks fine, so ACK. However what platforms don't have dlopen in -ldl? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Il giorno mer, 24/02/2010 alle 15.24 +0000, Richard W.M. Jones ha scritto:
The autoconf change looks fine, so ACK. However what platforms don't have dlopen in -ldl?
*BSD for instance -- Diego Elio Pettenò — “Flameeyes” http://blog.flameeyes.eu/ If you found a .asc file in this mail and know not what it is, it's a GnuPG digital signature: http://www.gnupg.org/
According to Richard W.M. Jones on 2/24/2010 8:24 AM:
On Wed, Feb 24, 2010 at 04:04:32PM +0100, Diego Elio Pettenò wrote:
Since on not all systems dlopen() is in libdl, add a very simple macro that allows to search for it and sets a DLOPEN_LIBS variable with its value to be used.
The autoconf change looks fine, so ACK. However what platforms don't have dlopen in -ldl?
In cygwin, dlopen is in -lc. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
With the recent changes to the linking defaults in Fedora 13 (namely enabling --no-add-needed behaviour by default), we have to pass the dlopen()-providing libraries directly at the link of the module; use the newly introduced macro to look for it and add it to the Makefile. --- configure.ac | 1 + src/Makefile.am | 1 + 2 files changed, 2 insertions(+), 0 deletions(-) diff --git a/configure.ac b/configure.ac index 4a848cd..804db90 100644 --- a/configure.ac +++ b/configure.ac @@ -303,6 +303,7 @@ fi AM_CONDITIONAL([WITH_OPENVZ], [test "$with_openvz" = "yes"]) if test "x$with_vbox" = "xyes"; then + AFX_LIB_DLOPEN AC_DEFINE_UNQUOTED([WITH_VBOX], 1, [whether VirtualBox driver is enabled]) fi AM_CONDITIONAL([WITH_VBOX], [test "$with_vbox" = "yes"]) diff --git a/src/Makefile.am b/src/Makefile.am index 46acb7d..87fe65c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -464,6 +464,7 @@ libvirt_driver_vbox_la_CFLAGS = \ if WITH_DRIVER_MODULES libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version endif +libvirt_driver_vbox_la_LIBADD = $(DLOPEN_LIBS) libvirt_driver_vbox_la_SOURCES = $(VBOX_DRIVER_SOURCES) endif -- 1.7.0
On Wed, Feb 24, 2010 at 04:04:33PM +0100, Diego Elio Pettenò wrote:
With the recent changes to the linking defaults in Fedora 13 (namely enabling --no-add-needed behaviour by default), we have to pass the dlopen()-providing libraries directly at the link of the module; use the newly introduced macro to look for it and add it to the Makefile. --- configure.ac | 1 + src/Makefile.am | 1 + 2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/configure.ac b/configure.ac index 4a848cd..804db90 100644 --- a/configure.ac +++ b/configure.ac @@ -303,6 +303,7 @@ fi AM_CONDITIONAL([WITH_OPENVZ], [test "$with_openvz" = "yes"])
if test "x$with_vbox" = "xyes"; then + AFX_LIB_DLOPEN AC_DEFINE_UNQUOTED([WITH_VBOX], 1, [whether VirtualBox driver is enabled]) fi AM_CONDITIONAL([WITH_VBOX], [test "$with_vbox" = "yes"]) diff --git a/src/Makefile.am b/src/Makefile.am index 46acb7d..87fe65c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -464,6 +464,7 @@ libvirt_driver_vbox_la_CFLAGS = \ if WITH_DRIVER_MODULES libvirt_driver_vbox_la_LDFLAGS = -module -avoid-version endif +libvirt_driver_vbox_la_LIBADD = $(DLOPEN_LIBS) libvirt_driver_vbox_la_SOURCES = $(VBOX_DRIVER_SOURCES) endif
ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://et.redhat.com/~rjones/libguestfs/ See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html
According to Diego Elio Pettenò on 2/24/2010 8:04 AM:
Check the host string before deciding whether to look for windres or not. dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""])
I like the idea. However, shouldn't you also guarantee that WINDRES is set even if $host is not windows-based, so that an inherited $WINDRES in a Linux environment does not cause spurious triggering of the AM_CONDITIONAL? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Feb 24, 2010 at 08:11:45AM -0700, Eric Blake wrote:
According to Diego Elio Pettenò on 2/24/2010 8:04 AM:
Check the host string before deciding whether to look for windres or not. dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""])
I like the idea. However, shouldn't you also guarantee that WINDRES is set even if $host is not windows-based, so that an inherited $WINDRES in a Linux environment does not cause spurious triggering of the AM_CONDITIONAL?
I'd argue the reverse, so users can set WINDRES in the environment before running configure, no? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 80 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
On 02/24/2010 04:22 PM, Richard W.M. Jones wrote:
On Wed, Feb 24, 2010 at 08:11:45AM -0700, Eric Blake wrote:
According to Diego Elio Pettenò on 2/24/2010 8:04 AM:
Check the host string before deciding whether to look for windres or not. dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""])
I like the idea. However, shouldn't you also guarantee that WINDRES is set even if $host is not windows-based, so that an inherited $WINDRES in a Linux environment does not cause spurious triggering of the AM_CONDITIONAL?
I'd argue the reverse, so users can set WINDRES in the environment before running configure, no?
That would try to add an icon to a Linux executable. I don't think it's the right thing to do, setting WINDRES in the environment _will_ work with Eric's proposed change when targeting Windows. Paolo
Just checking for a windres tool might hit even on Linux systems when building for Linux (e.g.: when using Gentoo and having built binutils with multitarget support), and will then fail to link properly at the end of the build. Check the host string before deciding whether to look for windres or not. --- configure.ac | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 117cb20..2e12f2e 100644 --- a/configure.ac +++ b/configure.ac @@ -1682,8 +1682,16 @@ AC_SUBST([CYGWIN_EXTRA_PYTHON_LIBADD]) AC_SUBST([MINGW_EXTRA_LDFLAGS]) dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; + *) + WINDRES="" + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""]) -- 1.7.0
According to Diego Elio Pettenò on 2/24/2010 8:44 AM:
-AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in
Redudant quotes. The shell word after case is never subject to field splitting, so it is sufficient to use: case $host in
+ *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; + *) + WINDRES=""
This works, but if it were me, I would remove the redundant quotes and write: WINDRES=
+ ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""])
But you now have my ACK, whether or not you strip the redundant ". -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Feb 24, 2010 at 04:04:30PM +0100, Diego Elio Pettenò wrote:
Just checking for a windres tool might hit even on Linux systems when building for Linux (e.g.: when using Gentoo and having built binutils with multitarget support), and will then fail to link properly at the end of the build.
Check the host string before deciding whether to look for windres or not. --- configure.ac | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac index 117cb20..5b1eb5f 100644 --- a/configure.ac +++ b/configure.ac @@ -1682,8 +1682,13 @@ AC_SUBST([CYGWIN_EXTRA_PYTHON_LIBADD]) AC_SUBST([MINGW_EXTRA_LDFLAGS])
dnl Look for windres to build a Windows icon resource. -AC_CHECK_TOOL([WINDRES], [windres], [no]) -AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != "no"]) +case "$host" in + *cygwin* | *mingw* ) + AC_CHECK_TOOL([WINDRES], [windres], []) + ;; +esac + +AM_CONDITIONAL([WITH_WIN_ICON], [test "$WINDRES" != ""])
From the POV of cross-compilation, this looks fine. ACK.
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
participants (5)
-
Diego Elio Pettenò -
Diego Elio “Flameeyes” Pettenò
-
Eric Blake -
Paolo Bonzini -
Richard W.M. Jones