Bug#884697: Progress

[Andreas Henriksson <andreas@fatal.se>]

Hello Christian Göttsche,

On Fri, Jun 29, 2018 at 12:34:44PM +0200, Christian Göttsche wrote:
> ping
> 
> I uploaded a new version (lintian fixes, new std version, updated vcs
> fields) to mentors (https://mentors.debian.net/package/logrotate).
> Someone any ideas about the piuparts issues I mentioned?
> 
> Otherwise I think the package is stable; its working for me on several machines.

I've looked over the 3.14.0-1 package version and generally everything
looks very good to me. I'm appending my review notes about minor things
below which might be useful for future improvements none the less.

Please tell me if you want me to go ahead with further testing and
uploading of the package, or if you already have someone else in mind
for this task.
Please also mention if you've contacted and what their response have
been for the people offering mentorship (like in 
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=887151#17 ).

----

# logrotate

WATCH OUT / HEADS UP:
- I'm not sure about the current state but this has bitten me in the
past. The debhelper systemd integration in the past had no particular
knowledge about timer units. That resulted in the service unit for the
respective timer unit being both enabled and *started* (or restarted,
depending on if the package is newly installed or upgraded) at package
installation/configure time. You likely do not want to trigger a
logrotate at package installation/upgrade time and delay the entire dpkg
operation until it completes. (I imagine some people might have massive
logs that might take a very long time.) Please verify if current
dh-systemd has improved on this front or if you need to add overrides
for logrotate.service to not be started/enabled. I suspect this might
very well be fixed now to just not start or enable services which don't
have any [Install] section (like logrotate.service, but adding a
build-time check to verify upstream doesn't slip one in there might be a
useful safety for the future?).


Minor things I reacted on that you might want to consider for future package
versions:


debian/upstream/metadata:
- Repository url should have '.git' appended instead of last '/', right?
- I think Bug-Database is more revelant for listing issues url instead
  of using Contact.

debian/control:
- I'm not sure using github project url in Homepage field is
  appropriate. It's supposed to be an url relevant for end users AFAIK.
  eg. github pages url would be suitable (if available, which it doesn't
  seem to be for logrotate).



debian/logrotate.preinst:
- how old is this? There are no version checks and I didn't look, but
  maybe it can be dropped now? The less manual written maintainerscript
  code the better.

debian/logrotate.README.Debian:
- this seems pretty outdated info as well considering for buster. Maybe
  it should also be dropped?

debian/rules:
- neat, but even better would be line-wrapping configure override using
  a backslash to put each config option on a separate line for easier
  reading.

debian/tests:
- given existance of tests reduces unstable->testing migration delay,
  this might just be a bit too trivial test to exist alone???
  (while at the same time an existing test might be better than no test
   at all....)

debian/patches/manpage.patch:
- why is this information relevant to put in the manpage? A more general
  solution would be to describe that apt-file can be used to search for
  which package contains something. Not suitable to document in
  specialized manpages like logrotate IMHO. Oh, I see this patch is not
  listed in series file so not applied. Well, might be another reason to
  drop it.

debian/patches:
- I see you've already upstreamed some of your work. I hope you will
  continue that trend with the remaining patches as well.



----

Regards,
Andreas Henriksson