[SSSD] [PATCH] use systemd by default on fedora15+

Jan Cholasta jcholast at redhat.com
Thu Nov 8 12:17:28 UTC 2012


Hi,

On 26.10.2012 16:00, Pavel Březina wrote:
> On 10/22/2012 03:46 PM, Jakub Hrozek wrote:
>> On Mon, Oct 22, 2012 at 02:00:29PM +0200, Pavel Březina wrote:
>>> On 10/19/2012 12:43 PM, Jakub Hrozek wrote:
>>>> On Fri, Oct 19, 2012 at 10:19:25AM +0200, Pavel Březina wrote:
>>>>> On 10/18/2012 03:24 PM, Simo Sorce wrote:
>>>>>> On Thu, 2012-10-18 at 09:46 +0200, Pavel Březina wrote:
>>>>>>> https://fedorahosted.org/sssd/ticket/1437
>>>>>>
>>>>>> Fedora F15 has been EOLed and will not get updates, why bother with
>>>>>> F15 ?
>>>>>> Also F15 systemd was quite immature, and at the time guidelines
>>>>>> mentioned you shouldn't change from sysv to systemd after GA.
>>>>>> Did you mean to put F16 there ?
>>>>>>
>>>>>> Simo.
>>>>>>
>>>>>
>>>>> OK. New patch is attached.
>>>>
>>>
>>> Hi,
>>>
>>>>>   %if (0%{?enable_experimental} == 1)
>>>>> @@ -57,6 +61,12 @@ Requires(post): initscripts chkconfig
>>>>>   Requires(preun):  initscripts chkconfig
>>>>>   Requires(postun): initscripts chkconfig
>>>>>
>>>>> +%if (0%{?use_systemd} == 1)
>>>>> +Requires(post): systemd-units
>>>>> +Requires(preun): systemd-units
>>>>> +Requires(postun): systemd-units
>>>>> +%endif
>>>>
>>>> I think here ^^ should be an %else that would otherwise Require
>>>> initscripts. I don't think we need them in a pure systemd-enabled
>>>> package.
>>>
>>> I'm no spec file expert, but everything is basically copy and paste
>>> from our Fedora spec file so I assume it should be correct.
>>>
>>> The Fedora spec file says:
>>> Requires(post): systemd-units initscripts chkconfig
>>
>> Right, I think the initscripts Requires is spurious,
>
> I'm sending new patch that requires initscripts conditionally.
>
> but we keep
>> chkconfig to migrate from sysv scripts (which Pavel just showed me is
>> forbidden by the Packaging Guidelines).
>

A few comments on the patch:


+Requires(post): chkconfig
+Requires(preun): chkconfig
+Requires(postun): chkconfig

Are you sure chkconfig is required in all of these? It is called only in 
%triggerun, so Requires(post) should be enough AFAIK.


+%if (0%{?use_systemd} == 1)
+# Replace sysv init script with systemd unit file
+rm -f $RPM_BUILD_ROOT/%{_initrddir}/%{name}
+mkdir -p $RPM_BUILD_ROOT/%{_unitdir}/
+cp src/sysv/systemd/sssd.service $RPM_BUILD_ROOT/%{_unitdir}/
+%endif

I think this should be done by calling configure with the right options 
instead:

%if (0%{?use_systemd} == 1)
     %global with_initscript --with-initscript=systemd 
--with-systemdunitdir=%{_unitdir}
%else
     %global with_initscript --with-initscript=sysv
%endif
...
%configure \
...
     %{with_initscript} \
...


+%preun
+if [ $1 = 0 ]; then

A nitpick: the test should read [ $1 -eq 0 ].


+%triggerun -- sssd < %{version}-%{release}
+if /sbin/chkconfig --level 3 sssd ; then
+        /bin/systemctl --no-reload enable sssd.service >/dev/null 2>&1 || :
+fi
+
+if /sbin/chkconfig --level 5 sssd ; then
+        /bin/systemctl --no-reload enable sssd.service >/dev/null 2>&1 || :
+fi
+
+/sbin/chkconfig --del sssd >/dev/null 2>&1 || :

I don't think this is right (both the sssd version and the body of the 
scriptlet), read 
<http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Packages_migrating_to_a_systemd_unit_file_from_a_SysV_initscript>.


+%postun
+/bin/systemctl daemon-reload >/dev/null 2>&1 || :
+if [ $1 -ge 1 ] ; then
+    # On upgrade, reload init system configuration if we changed unit files
+    /bin/systemctl daemon-reload >/dev/null 2>&1 || :
+    /bin/systemctl try-restart sssd.service >/dev/null 2>&1 || :
+fi

There is no reason to call systemctl daemon-reload again inside the if.


Honza

-- 
Jan Cholasta



More information about the sssd-devel mailing list