Re: [libvirt] PATCH: 2/4: Restructure sources files in Makefile.am
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/