On 10/24/2012 04:37 AM, Václav Pavlín wrote: > Hi, > > I created patch for spec file to follow changes in guidelines with > respect to new systemd macros (see > https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd) > > > new-systemd-macros.patch > > > diff --git a/libvirt.spec b/libvirt.spec > index b47ec0e..675290f 100644 > --- a/libvirt.spec > +++ b/libvirt.spec Upstream, we would want to patch libvirt.spec.in (the parent of libvirt.spec). > @@ -316,7 +316,7 @@ > Summary: Library providing a simple virtualization API > Name: libvirt > Version: 0.10.2 > -Release: 3%{?dist}%{?extra_release} > +Release: 4%{?dist}%{?extra_release} This is a downstream-only change, since upstream libvirt.spec.in does not track downstream releases. Besides, downstream is more likely to use the upstream v0.10.2-maint branch and use 0.10.2.1-1 as the next build for Fedora 18. > %if %{with_systemd} > -if [ $1 -eq 1 ] ; then > - # Initial installation > - /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : > - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : > -fi > +%systemd_post libvirtd.service cgconfig.service Hmm - we may have a pre-existing issue: this is a case of enabling a service that we don't own, but where cgconfig.service is a prerequisite of libvirtd.service. Is this really the right order for enabling the two services, and is it really necessary for use to explicitly enable cgconfig.service, or should the contents of libvirtd.service be adequate for systemd to automatically figure out the dependency? > > %changelog > +* Fri Oct 19 2012 Václav Pavlín <vpavlin(a)redhat.com&gt; - 0.10.2-4 > +- Scriptlets replaced with new systemd macros (#850186) Again, %changelog is something only needed for downstream builds. Also, your patch is missing a conversion for libvirt-guests.service, also in the spec file: $ grep -C2 libvirt-guests.service libvirt.spec.in # If the package is allowed to autostart: /bin/systemctl --no-reload enable libvirt-guests.service >/dev/null 2>&1 ||: # Run these because the SysV package being removed won't do them /sbin/chkconfig --del libvirt-guests >/dev/null 2>&1 || : /bin/systemctl try-restart libvirt-guests.service >/dev/null 2>&1 || : %endif -- %{_sysconfdir}/rc.d/init.d/libvirt-guests %if %{with_systemd} %{_unitdir}/libvirt-guests.service %endif %config(noreplace) %{_sysconfdir}/sysconfig/libvirt-guests

On Fri, Oct 26, 2012 at 08:09:54AM +0200, Václav Pavlín wrote: > Eric Blake píše v Čt 25. 10. 2012 v 10:41 -0600: >> Hello, > >> Ah, then maybe you did answer the big question. Except that I don't see >> the updated patch - did you forget to attach it? > > Sure, I did forgot, sorry. Here it is... > > diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in > index b7afadf..b78c301 100644 > --- a/daemon/libvirtd.service.in > +++ b/daemon/libvirtd.service.in > @@ -7,6 +7,7 @@ > Description=Virtualization daemon > Before=libvirt-guests.service > After=network.target > +Requires=cgconfig.service I explicitly left that out when i created the libvirtd.service file. Since systemd itself is capable of setting up cgroups by default, the common need for cgconfig.service has gone away. A minority of people may still wish to use it, but they can customize the libvirtd.service file themselves if desired - we shouldn't force cgconfig.service onto everyone esle.

> Then why are we explicitly starting cgconfig.service in the spec file? > Shouldn't the argument go that only those people customizing the service > file to use cgconfig need cgconfig enabled in the first place? In other > words, is there anything wrong with this one-liner? Oh that's probably a mistake then. We shouldn't be enabling it for systemd based installations. > > diff --git i/libvirt.spec.in w/libvirt.spec.in > index ebebfab..9d11328 100644 > --- i/libvirt.spec.in > +++ w/libvirt.spec.in > @@ -1465,13 +1465,12 @@ done > %endif > > %if %{with_systemd} > if [ $1 -eq 1 ] ; then > # Initial installation > /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : > - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : > fi > %else > %if %{with_cgconfig} > # Starting with Fedora 16/RHEL-7, systemd automounts all cgroups, > # and cgconfig is no longer a necessary service. > %if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?fedora} && 0%{?fedora} < 16) ACK