[libvirt] RPM spec file patch

[Eric Blake]

On 11/15/2011 11:42 PM, Chris Picton wrote:
> Hi
> 
> Please accept the following patch to the rpm spec file.
> 
> It allows me to enable specific options (like openvz) at the build
> comand line, even if they have been disabled by OS feature selection.
> 
> eg for an openvz build on centos 6
> 
> rpmbuild -bb \
>   --define 'rhel 6' \
>   --without dtrace \
>   --without sanlock \
>   --without netcf \
>   --with openvz \
> libvirt.spec
> 
> Regards

Thanks for the report.  However, I'm not yet quite convinced that your
proposed patch is the best approach.  I'll explain, focusing on just the
nwfilter macros as an example.

First, some background, to make sure I'm analyzing things correctly (and
in part, documenting things I learned so that I can refer to this mail
later :).
https://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html-single/RPM_Guide/index.html#id570231
was handy in seeing how spec file macros are expanded.  I temporarily
added this line to libvirt.spec, then ran variations of 'rpmbuild -bp
libvirt.spec' to see what effect things have:

%{error: with %{_with_nwfilter}, without %{_without_nwfilter}}

It was pretty easy to learn that:

rpmbuild --without a --with b

is short-hand for

rpmbuild --define '_without_a --without-a1' --define '_with_b --with-b'

Note that these are macros with a leading underscore, and that the
definition chosen happens to be one that can be reused as a typical
./configure argument.  But we later want to make decisions on
dependencies based on which commands were enabled via rpmbuild or
earlier %define, and checking both _without_nwfilter and _with_nwfilter
at every decision point is tedious, so we create a new macro,
with_nwfilter, whose contents are guaranteed to be a numeric string that
is usable as an %if expression (the value of the string is important
only as 0 vs. non-zero).


Now, notice that the existing spec file starts with:

%define with_nwfilter      0%{!?_without_nwfilter:0}

which guarantees with_nwfilter will be defined, either to "0" (if
_without_nwfilter is defined) or "00" (if _without_nwfilter was not
defined).  At first glance, that seems a bit fishy - shouldn't we be
defining it to 0 or 1, instead of 0 or 0?  But reading the comment makes
it clearer (this is setting a default to off, which is enabled later
based on other conditions); and indeed, the next use is:

%if %{with_qemu}
%define with_nwfilter 0%{!?_without_nwfilter:%{server_drivers}}
%endif

Aha.  If %{with_qemu} is set, then we redefine with_nwfilter; this time
to the value 0 if _without_nwfilter is set, or to 0%{server_drivers}
(which in turn is 00 or 01) if _without_nwfilter is missing.  If
%{with_qemu} is clear, then we leave with_nwfilter at 0.

After that point, your patch would then add:

%{?_with_nwfilter:%define with_nwfilter 1}

which says that if _with_nwfilter is defined, then redefine
with_nwfilter once more, this time to 1.

Still, that's a lot of logic to go through three %defines before we get
to the final value.  Also, it looks like our spec file does not make it
easy to override client_only, which controls several of the defaults (of
course, it is okay to hardcode client_only to 1 on the architectures
where we do not support libvirtd, but allowing the user to override it
to 0 on platforms where we normally build the server would be handy).

Perhaps it would be simpler to do something like this, which 1) fixes
client_only to allow user override on appropriate platforms, and 2) sets
default values based on inspecting _both_ with and without variants.
This is not a complete patch, so much as a request for further
discussion.  The idea is to demonstrate how defining the defaults for
with_libvirtd and with_polkit should consider the _with_ variant in
addition to the _without_ variant; again with the end result being 0 to
disable, nonzero (whether 010, 011, or 001) to enable.

Thoughts?

diff --git i/libvirt.spec.in w/libvirt.spec.in
index 89f710c..d085feb 100644
--- i/libvirt.spec.in
+++ w/libvirt.spec.in
@@ -11,7 +11,7 @@
 # A client only build will create a libvirt.so only containing
 # the generic RPC driver, and test driver and no libvirtd
 # Default to a full server + client build
-%define client_only        0
+%{!?client_only:%define client_only        0}

 # Now turn off server build in certain cases

@@ -34,7 +34,7 @@
 # of any particular OS

 # First the daemon itself
-%define with_libvirtd      0%{!?_without_libvirtd:%{server_drivers}}
+%define with_libvirtd
0%{!?_without_libvirtd:%{server_drivers}%{?_with_libvirtd:1}}
 %define with_avahi         0%{!?_without_avahi:%{server_drivers}}

 # Then the hypervisor drivers that run on local host
@@ -64,7 +64,7 @@
 %define with_selinux       0%{!?_without_selinux:%{server_drivers}}

 # A few optional bits off by default, we enable later
-%define with_polkit        0%{!?_without_polkit:0}
+%define with_polkit        0%{!?_without_polkit:0}%{?_with_polkit:1}
 %define with_capng         0%{!?_without_capng:0}
 %define with_netcf         0%{!?_without_netcf:0}
 %define with_udev          0%{!?_without_udev:0}


-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111116/caf7b45e/attachment-0001.sig>