[libvirt] [PATCH] vbox: Let configure detect/set the XPCOMC directory

[Eric Blake]

On 06/29/2010 12:27 PM, Matthias Bolte wrote:
> This allows the user to give an explicit path to configure
> 
>   ./configure --with-vbox=/path/to/virtualbox
> 
> instead of having the VirtualBox driver probe a set of possible
> paths at runtime. If no explicit path is specified then configure
> probes the set of "known" paths.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=609185

> -  AC_HELP_STRING([--with-vbox], [add VirtualBox support @<:@default=yes@:>@]),[],[with_vbox=yes])
> +  AC_HELP_STRING([--with-vbox=@<:@PFX@:>@], [VirtualBox XPCOMC location @<:@default=check@:>@]),[],[with_vbox=check])

As long as you're touching this line, let's wrap at 80 columns (after
any "]," sequence is a good space for a line break, since m4 ignores
leading whitespace prior to the next "[").

> +
> +    if test -f /usr/lib/virtualbox/VBoxXPCOMC.so; then
> +        vbox_xpcomc_dir=/usr/lib/virtualbox
> +    else
> +        if test -f /usr/lib/VirtualBox/VBoxXPCOMC.so; then
> +            vbox_xpcomc_dir=/usr/lib/VirtualBox
> +        else
> +            if test -f /opt/virtualbox/VBoxXPCOMC.so; then

That gets long, fast.  My first reaction was suggesting to use
if...elif...elif...fi rather than nested if...fi, to avoid insane
indentation levels.  But even that's long - a for loop is better (and
easier maintenance - just add one line for any new potential spelling):

for vbox in \
  /usr/lib/virtualbox/VBoxXPCOMC.so \
  /usr/lib/VirtualBox/VBoxXPCOMC.so \
...
  /Application/VirtualBox.app/Contents/MacOS/VBoxXPCOMC.dylib \
  ; do
  if test -f "$vbox"; then
    vbox_xpcomc_dir=AS_BASENAME(["$vbox"])
    break
  fi
done

> +
> +    if test -n "$vbox_xpcomc_dir"; then
> +        AC_MSG_RESULT([$vbox_xpcomc_dir])
> +        with_vbox=yes
> +    else
> +        if test "x$with_vbox" = "xcheck"; then
> +            AC_MSG_RESULT([not found, disabling VirtualBox driver])
> +            with_vbox=no
> +        else
> +            AC_MSG_RESULT([not found])
> +            AC_MSG_ERROR([VirtualBox XPCOMC is required for the VirtualBox driver])
> +        fi
> +    fi
> +else
> +    if test "x$with_vbox" != "xno"; then
> +        if test -f ${with_vbox}/VBoxXPCOMC.so || test -f ${with_vbox}/VBoxXPCOMC.dylib; then
> +            vbox_xpcomc_dir=$with_vbox
> +            with_vbox=yes
> +        else
> +            AC_MSG_ERROR([$with_vbox does not contain VirtualBox XPCOMC])
> +        fi
> +    fi

This part looks good.

> +fi
> +
> +AC_SUBST([vbox_xpcomc_dir])

Wouldn't AC_DEFINE_UNQUOTED([VBOX_XPCOMC_DIR], ["$vbox_xpcomc_dir"]) be
better, so that the definition automatically goes into config.h...

> +
>  if test "x$with_vbox" = "xyes"; then
>      AC_SEARCH_LIBS([dlopen], [dl], [], [AC_MSG_ERROR([Unable to find dlopen()])])
>      case $ac_cv_search_dlopen in
> diff --git a/src/Makefile.am b/src/Makefile.am
> index ece18a6..d3c7087 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -19,6 +19,7 @@ INCLUDES =							\
>  		-DPKGDATADIR=\""$(pkgdatadir)"\"		\
>  		-DLOCAL_STATE_DIR=\""$(localstatedir)"\"	\
>  		-DGETTEXT_PACKAGE=\"$(PACKAGE)\"		\
> +		-DVBOX_XPCOMC_DIR=\"$(vbox_xpcomc_dir)\"		\

...rather than requiring a longer makefile command line?  The reason
that other directory variables are substituted at make time (like
$(pkgdatadir)) is because GNU Coding Standards requires that they be 1)
built in terms of other expansions (so you need multiple levels of
expansion) and 2) replaceable at make time (so you can do make
pkgdatadir=/alt/path).  But vbox_xpcomc_dir fits neither of these
situations (we aren't building it in terms of $(prefix), because we
hardcoded the full directory name where we found it, and it is not
something that should be replaceable at make time to a different location).

> -#elif defined(__FreeBSD__)
> -    if (tryLoadOne("/usr/local/lib/virtualbox", 1) == 0)
> +
> +    if (tryLoadOne(VBOX_XPCOMC_DIR, 1) == 0)

This also looks nicer.

I like the idea, but think we need v2.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100629/c4dd446d/attachment-0001.sig>