[libvirt] PATCH: 2/4: Restructure sources files in Makefile.am

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/