On 3 November 2015 at 17:48, Bryan Quigley <bryan.quig...@canonical.com> wrote:
> On Mon, Nov 2, 2015 at 5:24 PM, Felipe Sateler <fsate...@debian.org> wrote:
>>>>> ++if [ -n "$sessions_found" ] ; then
>>>>> ++ echo "done."
>>>>> ++else
>>>>> ++ echo "none found."
>>>>> ++fi
>>>>
>>>> This is a behavior change: previously the recover script would not
>>>> print any output. Maybe this should not be printed?
>>>
>>> The previous init script did print none found though.
>>
>> Maybe I'm worrying too much (I leave it to you to decide), but maybe
>> people are using this in cronjobs or other scripted environments, and
>> this changes the output.
>
> I don't think there's an obvious win either way. Either it's closer
> the previous init script or to the previous recover script.
>
>>
>>>
>>>> Why do you invoke dh_systemd_enable manually? the --with=systemd above
>>>> should do it.
>>>>
>>>>> + dh_installinit -- start 70 S .
>>>>
>>>> start argument is obsolete now and just invokes `defaults`, so this
>>>> override can go.
>>>
>>> Because I didn't know what i was doing, oops.
>>
>> You removed the --no-start --no-restart-on-upgrade arguments to
>> dh_systemd_start, are you sure you want to restart on package
>> install/upgrade?
>
> Yea, I was thinking about that. Might as well let the script run on
> package upgrade. It shouldn't hurt anything and it might clear out
> old files before an upgrade on machines that haven't been restarted
> recently.
>
>>>> However, this changes the name of the init script from nviboot to nvi.
>>>> This means you need to use dpkg-maintscript-helper to handle the
>>>> rename, and remove the old enablement links as well. If you want to do
>>>> this you might as well move it out of runlevel S ;)
>>>
>>> Moved out of runlevel S and it deletes nviboot correctly now.
>>
>> Cool. Except for 3 things:
>>
>> 1. Instead of adding the rm_connfile calls maually, you can use the
>> dh_installdeb support. Check the manpage for details (basically, put
>> the rm_conffile part only once in debian/nvi.maintscript).
>
> Heh. that's much quicker. Thanks!
You need to remove the -- "$@" part: it is being added twice to the
resulting scripts (check debian/nvi/DEBIAN/{pre,post}* after a build):
dpkg-maintscript-helper rm_conffile /etc/init.d/nviboot 1.81.6-12 nvi
-- "$@" -- "$@"
>>
>> 2. This is not really a rm_conffile, but rather a mv_conffile (the
>> script is renamed, not removed).
>
> Really the init get's split up to nvi and the recover script. If the
> modified the recover script logic the mv_conffile wouldn't help them.
Indeed.
>
>>
>> 3. This will not preserve enable/disable modifications by the user.
>> There is no simple solution for this (you need to check in preinst the
>> state if differing from the default, and restore it in postinst for
>> the new script name), so maybe just adding a note to NEWS.Debian
>> suffices? Any modifications made to the init script will be lost
>> anyways when using systemd as then only the service file will be used.
>
> Added to to NEWS.Debian.
Great.
>
>>Also, you might want to remove the - from the su call. A login shell is not
>>require
> Done.
>
>> Thanks for your work!
> Thanks for all your work reviewing this! I learned a bunch.
After the above change I think this would be ready for upload. Can you
upload or do you need sponsoring? I can do the actual upload.
--
Saludos,
Felipe Sateler
