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

On 06/29/2010 12:27 PM, Matthias Bolte wrote: 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 "["). 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 This part looks good. Wouldn't AC_DEFINE_UNQUOTED([VBOX_XPCOMC_DIR], ["$vbox_xpcomc_dir"]) be better, so that the definition automatically goes into config.h... ...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). This also looks nicer. I like the idea, but think we need v2. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org