On 03/30/2012 10:53 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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?
But I can live with them here.
...
> +# 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.
I spotted a few things you can clean up, but nothing was a show-stopper
to pushing this as-is, so you have my ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list