On Wed, Aug 13, 2008 at 03:17:09PM +0100, Daniel P. Berrange wrote:
Over time we've added lots of general purpose source code files, as wel as making more of the existing ones conditionally compiled. We've done this by adding lots of #ifdef WITH_XXXX macros across the various source files. This is becoming rather a minefield with soo many conditionals sprinkled throughout the code.
With all the recent re-factoring we now have very good separation between generic code, and driver specific code - each typically being in separate files. Thus it is now practical to remove the vast majority of the macros and just do the conditional compilation via the Makefile.am.
The patch is not entirely clear - the resulting Makefile.am is much easier to review. It is structured in two section, first we declare variables for groups of source code files - eg generic helper files, generic domain XML files, generic network XML files, and then per-driver files.
In general this sounds good, I don't like automake conditionals too much, but in that case yes, that's logical. [...]
In removing the unneeded WITH_XXX macros from header/sources I noticed that a bunch of our header files have
#ifdef __cplusplus extern "C" { #endif
With is irrelevant since we're not using C++ anywhere - indeed some of the files even had the corresponding '}' missing, so clearly this is never actually used during compilation. Removing this fixes bogus indentation in some of the header files
Well it should be kept in the public headers, but right internally it's just remains of cut and paste.
I've tested this all by running configure --without-XXX for each hypervisor driver in turn, and it seemed to work in all cases. The only thing I didn't test is MinGW. I think I really need to add a MinGW based build to the nightly build system, so we can sanity check that nightly --- a/configure.in Tue Aug 12 22:21:25 2008 +0100 +++ b/configure.in Tue Aug 12 22:21:47 2008 +0100 @@ -241,27 +241,35 @@ LIBVIRT_FEATURES= WITH_XEN=0
-if test "$with_openvz" = "yes" ; then +if test "$with_openvz" = "yes"; then
hum, unrelated :-)
-if test "$with_qemu" = "yes" ; then +if test "$with_qemu" = "yes" -o "$with_lxc" = "yes" ; then AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],, AC_MSG_ERROR([You must install kernel-headers in order to compile libvirt])) fi oh, good point !
[...]
+if WITH_REMOTE +libvirt_la_SOURCES += $(REMOTE_DRIVER_SOURCES) +endif
I'm wondering about += portability, but I think this is widely used and really clarifies the resulting Makefile.am the other solution libvirt_la_SOURCES = .... if WITH_REMOTE $(REMOTE_DRIVER_SOURCES) endif if WITH_XEN .... might be a bit more portable but messes the structure, No other comment, everything else is small bugs, the cleanup of headers and reindent, Fine by me, +1 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/