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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list