On 11/15/2011 11:42 PM, Chris Picton wrote: 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-... 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org