Hi Bryan, On Thu, 17 Sep 2015 11:45:25 -0400 Bryan Quigley <bryan.quig...@canonical.com> 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.init 1969-12-31 19:00:00.000000000 -0500 > +++ nvi-1.81.6/debian/nvi.init 2015-09-16 22:54:43.000000000 -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.000000000 -0500 > +++ nvi-1.81.6/debian/nvi.service 2015-09-16 22:54:51.000000000 -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.000000000 -0500 > +++ nvi-1.81.6/debian/patches/30make_recover_script_init_ready.patch > 2015-09-17 11:10:01.000000000 -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 <bryan.quig...@canonical.com> > + > +--- > +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 >/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 should not be printed? > ++ > ++ > diff -Nru nvi-1.81.6/debian/patches/series nvi-1.81.6/debian/patches/series > --- nvi-1.81.6/debian/patches/series 2015-09-16 10:16:11.000000000 -0400 > +++ nvi-1.81.6/debian/patches/series 2015-09-17 10:10:36.000000000 -0400 > @@ -25,3 +25,4 @@ > 26trailing_tab_segv.patch > 27support_C_locale.patch > 29file_backup.patch > +30make_recover_script_init_ready.patch > diff -Nru nvi-1.81.6/debian/rules nvi-1.81.6/debian/rules > --- nvi-1.81.6/debian/rules 2015-09-16 10:16:11.000000000 -0400 > +++ nvi-1.81.6/debian/rules 2015-09-16 23:06:45.000000000 -0400 > @@ -18,7 +18,7 @@ > endif > > %: > - dh $@ > + dh $@ --with=systemd > > override_dh_auto_clean: > rm -f dist/recover > @@ -70,4 +70,6 @@ > dh_compress -Xvi.advanced -Xvi.beginner > > override_dh_installinit: > - dh_installinit --init-script=nviboot -- start 70 S . > + dh_systemd_enable 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. 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 ;) > + dh_systemd_start --no-start --no-restart-on-upgrade This one is ok to override, but should be done in override_dh_systemd_start Saludos,
