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
