Bug#796635: Systemd job

[Felipe Sateler]

On 3 November 2015 at 20:19, Felipe Sateler  wrote:
> On 3 November 2015 at 18:15, Bryan Quigley  
> wrote:
>>> 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
>>> -- "$@" -- "$@"
>>>
>>
> 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.
>>
>> I need sponsoring.
>
> OK, I will take a look at uploading over the weekend.

Uploaded with minor changelog edit:

 * QA upload
 * Add systemd job (Closes: #796635)
 * Make recover script complete enough for use in init process
   - Rename init script from nviboot to nvi, move out of runlevel S
 * Cleanup


-- 

Saludos,
Felipe Sateler

Bug#796635: Systemd job

[Felipe Sateler]

On 2 Nov 2015 18:58, "Bryan Quigley"  wrote:
>
> Thanks for the review Felipe
>
> > Why did you preserve runlevel S? I don't think this really belongs in
> > recovery mode.
> Changed
>
> >> +;;
> >> +  stop|restart|reload|force-reload)
> >
> > restart (and force-reload?) should probably re-run the recovery script.
>
> Changed.
>
>
> >> +Description=To recover nvi edit sessions.
> >
> > s/To r/R/ ?
> +1
>
> > Also, the init script Provides: nviboot, so you should provide a link
> > from nviboot.service to nvi.service in /lib/systemd/system.
> I changed it so it provided nvi.  That was the whole reason I changed
> the name afterall.
>
> >> ++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.
>
> > 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.
>
> > 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.
>

Also, you might want to remove the - from the su call. A login shell is not
required.

Saludos

Bug#796635: Systemd job

[Felipe Sateler]

On 3 November 2015 at 17:48, Bryan Quigley  wrote:
> On Mon, Nov 2, 2015 at 5:24 PM, Felipe Sateler  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

Bug#796635: Systemd job

[Bryan Quigley]

> 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
> -- "$@" -- "$@"
>

>>> 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.

I need sponsoring.

Thanks!
Bryan


nvi_1.81.6-12.debdiff
Description: Binary data

Bug#796635: Systemd job

[Bryan Quigley]

On Mon, Nov 2, 2015 at 5:24 PM, Felipe Sateler  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!
>
> 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.

>
> 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.

>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.

Kind regards,
Bryan


nvi_1.81.6-12.debdiff
Description: Binary data

Bug#796635: Systemd job

[Felipe Sateler]

On 3 November 2015 at 18:15, Bryan Quigley  wrote:
>> 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
>> -- "$@" -- "$@"
>>
>
 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.
>
> I need sponsoring.

OK, I will take a look at uploading over the weekend.


-- 

Saludos,
Felipe Sateler

Bug#796635: Systemd job

[Bryan Quigley]

Thanks for the review Felipe

> Why did you preserve runlevel S? I don't think this really belongs in
> recovery mode.
Changed

>> +;;
>> +  stop|restart|reload|force-reload)
>
> restart (and force-reload?) should probably re-run the recovery script.

Changed.


>> +Description=To recover nvi edit sessions.
>
> s/To r/R/ ?
+1

> Also, the init script Provides: nviboot, so you should provide a link
> from nviboot.service to nvi.service in /lib/systemd/system.
I changed it so it provided nvi.  That was the whole reason I changed
the name afterall.

>> ++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.

> 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.

> 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.

Thanks again,
Bryan


nvi_1.81.6-12.debdiff
Description: Binary data

Bug#796635: Systemd job

[Felipe Sateler]

On 2 November 2015 at 18:57, Bryan Quigley  wrote:
> Thanks for the review Felipe
>
>> Why did you preserve runlevel S? I don't think this really belongs in
>> recovery mode.
> Changed
>
>>> +;;
>>> +  stop|restart|reload|force-reload)
>>
>> restart (and force-reload?) should probably re-run the recovery script.
>
> Changed.
>
>
>>> +Description=To recover nvi edit sessions.
>>
>> s/To r/R/ ?
> +1
>
>> Also, the init script Provides: nviboot, so you should provide a link
>> from nviboot.service to nvi.service in /lib/systemd/system.
> I changed it so it provided nvi.  That was the whole reason I changed
> the name afterall.

OK.

>
>>> ++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.

>
>> 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?

>
>> 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).

2. This is not really a rm_conffile, but rather a mv_conffile (the
script is renamed, not removed).

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.


Thanks for your work!

-- 

Saludos,
Felipe Sateler

Bug#796635: Systemd job

[Felipe Sateler]

Hi Bryan,

On Thu, 17 Sep 2015 11:45:25 -0400 Bryan Quigley
 wrote:
> Here is a debdiff that implements a systemd unit.
>
> (This is the first unit I've written, so review definitely needed)

Thanks for the patch. I agree with your previous comment that this
should maybe be a cronjob instead of boot script, but I get it that
you may want to keep the changes relatively minimal.

 Some comments:

> diff -Nru nvi-1.81.6/debian/nvi.init nvi-1.81.6/debian/nvi.init
> --- nvi-1.81.6/debian/nvi.init1969-12-31 19:00:00.0 -0500
> +++ nvi-1.81.6/debian/nvi.init2015-09-16 22:54:43.0 -0400
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +### BEGIN INIT INFO
> +# Provides:  nviboot
> +# Required-Start:$remote_fs
> +# Required-Stop:
> +# Default-Start: S

Why did you preserve runlevel S? I don't think this really belongs in
recovery mode.

> +# Default-Stop:
> +# Short-Description: Script to recover nvi edit sessions.
> +### END INIT INFO
> +
> +. /lib/lsb/init-functions
> +
> +case "$1" in
> +  start)
> +/usr/share/vi/recover

+1 on moving the logic outside of init script.

> +;;
> +  stop|restart|reload|force-reload)

restart (and force-reload?) should probably re-run the recovery script.

> +;;
> +esac
> +
> +exit 0
> diff -Nru nvi-1.81.6/debian/nvi.service nvi-1.81.6/debian/nvi.service
> --- nvi-1.81.6/debian/nvi.service 1969-12-31 19:00:00.0 -0500
> +++ nvi-1.81.6/debian/nvi.service 2015-09-16 22:54:51.0 -0400
> @@ -0,0 +1,9 @@
> +[Unit]
> +Description=To recover nvi edit sessions.

s/To r/R/ ?

> +
> +[Service]
> +Type=oneshot
> +ExecStart=/usr/share/vi/recover
> +
> +[Install]
> +WantedBy=multi-user.target

This will start after basic boot, which is good. I think the init
script should be changed to match this.

Also, the init script Provides: nviboot, so you should provide a link
from nviboot.service to nvi.service in /lib/systemd/system.

> diff -Nru nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch 
> nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch
> --- nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch  
> 1969-12-31 19:00:00.0 -0500
> +++ nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch  
> 2015-09-17 11:10:01.0 -0400
> @@ -0,0 +1,72 @@
> +Description: Changes recover script so it can be run by init
> +This was mostly just merging code from the previous init script
> +Author: Bryan Quigley 
> +
> +---
> +Bug-Debian: https://bugs.debian.org/796635
> +Bug-Ubuntu: https://launchpad.net/bugs/1489939
> +Forwarded: no
> +Last-Update: <2015-09-17>
> +
> +--- nvi-1.81.6.orig/dist/recover.in
>  nvi-1.81.6/dist/recover.in
> +@@ -8,11 +8,18 @@ RECDIR="@vi_cv_path_preserve@"
> + SENDMAIL="@vi_cv_path_sendmail@"
> +
> + echo 'Recovering nvi editor sessions.'
> ++sessions_found=""
> +
> + # Check editor backup files.
> + vibackup=`echo $RECDIR/vi.*`
> + if [ "$vibackup" != "$RECDIR/vi.*" ]; then
> + for i in $vibackup; do
> ++# Discard symlinks
> ++if test -L $i ; then
> ++rm -f $i
> ++continue
> ++fi
> ++
> + # Only test files that are readable.
> + if test ! -r $i; then
> + continue
> +@@ -36,6 +43,12 @@ fi
> + virecovery=`echo $RECDIR/recover.*`
> + if [ "$virecovery" != "$RECDIR/recover.*" ]; then
> + for i in $virecovery; do
> ++# Discard symlinks
> ++if test -L $i ; then
> ++rm -f $i
> ++continue
> ++fi
> ++
> + # Only test files that are readable.
> + if test ! -r $i; then
> + continue
> +@@ -49,11 +62,21 @@ if [ "$virecovery" != "$RECDIR/recover.*
> + # Delete any recovery files that are zero length, corrupted,
> + # or that have no corresponding backup file.  Else send mail
> + # to the user.
> +-recfile=`awk '/^X-vi-recover-path:/{print $2}' < $i`
> +-if test -n "$recfile" -a -s "$recfile"; then
> +-$SENDMAIL -t < $i
> +-else
> +-rm $i
> +-fi
> ++recfile=`awk '/^X-vi-recover-path:/{print $2}' < $i`
> ++if test -n "$recfile" -a -s "$recfile"; then
> ++sessions_found="yes"
> ++owner=`stat --format='%U' $recfile`
> ++(su - nobody -s /bin/sh -c "$SENDMAIL $owner < $i" 
> &) /dev/null 2>&0
> ++else
> ++rm $i
> ++fi
> + done
> + fi
> ++
> ++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 

Bug#796635: Systemd job

[Bryan Quigley]

Here is a debdiff that implements a systemd unit.

(This is the first unit I've written, so review definitely needed)


nvi_1.81.6-12.debdiff
Description: Binary data