On Thu, Apr 04, 2019 at 10:20:47AM +0000, Dmitry Bogatov wrote:
> * Please, do not use ${runit:Conflicts}. As suggested by Lintian, it is
> very strong relations. Use ${runit:Breaks} only. The very
> introduction of ${runit:Conflicts} was my mistake. It will make
> Lintian override unneeded.
Will do.
> * Please, consider merging bin:socklog-run into bin:socklog. Option
> `since' for dh_runit will be useful for it.
Ah that's what you meant, sorry I misunderstood. I will try to merge
them.
> * I know, it is pain, but there should be init.d script. You may want to
> take a look at bcron=0.11-8.
Sure, no worries. How about systemd service files? It makes little sense
to run socklog along with systemd I think, but for the principle it may
be required to profile service files. What do you think?
> * Please, add description to 0002-import-patch-tryto. It is unclear,
> what issue this patch resolves.
This one comes from the previous packaging scripts. I'll do some
digging.
> * In patch 0003-remove-chkshgrp you remove test, that fails on CI. Does
> it also fails in sbuild? If not, probably it should only be disabled in
> Gitlab CI?
It only fails in CI. I'll try to have it disabled in CI only, that would
be much better indeed.
> * It is matter of taste, but are you aware, that you can
>
> Build-Depends: debhelper-compat (= 12)
>
> and remove `debian/compat'?
I didn't know, thanks :)
> * Dep-5 would be nice.
Will do.
> * What is the purpose of 'log' user, you create with dh_sysuser? You
> know, that bin:runit provides user `runit-log' since -20, don't you?
I overlooked that, thanks.
> * All services run as same user, 'nobody'. It is unfortunate, since nobody is
> quite popular owner for NFS files.
>
> I believe there should be separate sysuser for socklog-* services.
> Ideally, separate sysuser for /every/ from socklog-* service, but I do
> not know, whehter it is possible.
Yeah good point. I tend to think that a single user for all socklog-*
services would be enough, but if you prefer I can add one user per
service.
> * I believe, README file is useless -- it contains copyright, authorship
> and homepage information only, which is already present in Debian
> package files.
Alright, I'll remove it.
Thanks for the review!
Cheers,
--
Mathieu Mirmont <mat@parad0x.org>
Attachment:
signature.asc
Description: PGP signature