Bug#884697: logrotate package
Hi Andreas,
thank you for taking some time and looking over the package.
> 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.
Thanks!
> 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.
Yes, I'd like to go ahead.
> 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 ).
I messaged them
(https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=887151#45) but got
unfortunately no response yet.
> 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?).
Thanks for the hint; I checked this and package installation/upgrade
does not start the service (only the timer).
I am not sure though how to write a build time check.
> 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.
https://salsa.debian.org/debian/logrotate/commit/46bc5f248aa7d1af886a7b413902e374486790c9
> 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).
There is unfortunately no homepage as such.
> 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.
Seems this was changed in 3.11. so I'd like to keep this for buster.
> debian/logrotate.README.Debian:
> - this seems pretty outdated info as well considering for buster. Maybe
> it should also be dropped?
https://salsa.debian.org/debian/logrotate/commit/0dee0d88c85ecfe5e74523f0de89a50379b5e508
> 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.
https://salsa.debian.org/debian/logrotate/commit/0b52887e1256f6895a1a227df5b96db5f19315fd
> 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....)
I do not know what the test environment offers, so I only wrote that
simple test.
In my opinion the build time tests plus the test that logrotate is
running (even only printing the version and build information) is
sufficient for now.
But I welcome any test ideas.
> 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.
https://salsa.debian.org/debian/logrotate/commit/d3f78773060c4dbc91812e8fcbd24322658e6a52
Best regards,
Christian Göttsche
Reply to: