Re: [libvirt] [PATCH] Refactor the libvirt RPM daemon pieces

On 03/30/2012 10:53 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > There are a number of flaws with our packaging of the libvirtd > daemon: > > + > +%if %{with_libvirtd} > +%package daemon > +Summary: Server side daemon and supporting files for libvirt library > +Group: Development/Libraries > + > +# All runtime requirements for the libvirt package (runtime requrements > +# for subpackages are listed later in those subpackages) > + > +# The client side, i.e. shared libs and virsh are in a subpackage > +Requires: %{name}-client = %{version}-%{release} > + > +# Used by many of the drivers, so turn it on whenever the > +# daemon is present > +%if %{with_libvirtd} You've now nested %if %{with_libvirtd} twice; this could be simplified, but cleanups can be saved for another day if we are under the gun to get this out now (is there a cppi equivalent for spec files to help show nesting? Can you even indent %if directives for sanity in a spec file?)

> +# for modprobe of pci devices ... > +# For glusterfs > +%if 0%{?fedora} >= 11 > +Requires: glusterfs-client >= 2.0.1 > +%endif > +%endif > +%if %{with_qemu} > +# From QEMU RPMs > +Requires: /usr/bin/qemu-img Should this Requires: be pushed down to libvirt-daemon-qemu...

> +# For image compression > +Requires: gzip > +Requires: bzip2 > +Requires: lzop > +Requires: xz > +%else > +%if %{with_xen} > +# From Xen RPMs > +Requires: /usr/sbin/qcow-create and this one to libvirt-daemon-kvm?

> +# For virConnectGetSysinfo > +Requires: dmidecode > +%endif > +# For service management > +%if %{with_systemd} > +Requires(post): systemd-units > +Requires(post): systemd-sysv > +Requires(preun): systemd-units > +Requires(postun): systemd-units These four requires deal with services, but we already pushed services into libvirt-daemon-config-network; I'm guessing these should be relocated to that section. Or maybe I'm misunderstanding - is it that these enable libvirtd to run as a service, but libvirtd will do nothing to cause interference (like trying to set up the 'default' NAT network at 192.168.122.0) unless you also have the config files? In that case, what you did looks okay.

> +%endif > +%if %{with_numad} > +Requires: numad > +%endif > + > +%description daemon > +Server side daemon required to manage the virtualization capabilities > +of recent versions of Linux. Requires a hypervisor specific sub-RPM > +for specific drivers. It looks weird to have the %description so far down from the %package with all the Requires in between. But that's cosmetic.