Bug#687526: Bug#696983: slbackup: cronjob warn about obsolete getopts.pl dependency
On 2013-01-07 12:12, Holger Levsen wrote:
> Hi Mike,
>
> On Dienstag, 1. Januar 2013, Mike Gabriel wrote:
>> package for wheezy is not possible. For wheezy, slbackup has to be
>> provided via the D-E archive. (Long story [1]). I'll keep the package
>> [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=687526
>
> now that I the bug report I must say: "what?" slbackup must surely be fixed in
> wheezy, not just in debian-edu. Mike, why do you gave up on 687526? You could
> have also asked for help :-)
>
> Anyway, my proposal for 0.0.12-5 for sid+wheezy:
>
Hi,
>From the loose review of the diff between sid and experimental, I don't
think I'd bother with reverting the changes. We accept translation
fixes if it goes together with another fix and the rest of the changes
are "mah, whatever" or simple doc fixes. Though, others may disagree
with those.
> [...]
>
> keep this change from 0.0.12-4:
> * Change in conffile management:
> + Drop CRON job /etc/cron.daily/slbackup. Re-enable configuration
> of slbackup via debconf templates (closes: #662914).
> + Remove conffile /etc/cron.daily/slbackup via dpkg−maintscript−helper.
>
> and add the fix 696983 and upload to sid.
>
What I am really concerned about is the postinst changes, which ignores
open/close return values (silent error hiding) and I'd would probably
change the line[1]:
+system("dpkg-maintscript-helper rm_conffile ...");
to use the list variant (ensures proper quoting). Btw, does it autodie
on system failures[2]? Otherwise you are not checking the return value
from dpkg-maintscript-helper (and possibly hiding an error here as well).
More nitpick:
+ if ( ! -e "/etc/cron.d/slbackup" and $enable eq "true") {
^^^^^^^^^^^^^^^^^^^^^
The marked part above...
+ # make cron-job
+ my $crontab = "# cron job for Skolelinux Backup (once a day)\n";
+ if ($enable eq "false") { $crontab .= "#"; }
^^^^^^^^^^^^^^^^^^
... appears to cause the marked condition to be a contradiction (i.e.
always false).
Mind you, I am a bit out of my conform zone on what this postinst script
is doing, so I am not sure I'd ACK it on my own (even with the parts
above fixed). Particularly, Julien already suggested that he found the
changes to invasive[3].
> Mike, what do you think? Can you prepare that, so that I can review+upload?
>
>
> cheers,
> Holger
>
>
[1] Replacement being (something like):
system ('dpkg-maintscript-helper', 'rm_conffile',
'/etc/cron.daily/slbackup 0.0.12-3̈́',
'--', @ARGV);
[2] http://search.cpan.org/~pjf/autodie-2.13/lib/autodie.pm
[3] <20121201113544.GH5634@radis.cristau.org>
Reply to: