Bug#848993: RFS: llmnrd/0.2-1 [ITP]
Hello,
Started looking at this bug report yesterday but got discracted...
I spotted much the same issues as Chrisian so I'll instead just
echo what he's saying and add a few comments.
(I'll be able to sponsor you once the package is ready.)
On Fri, Dec 23, 2016 at 12:12:17AM +0100, Christian Seiler wrote:
> Hi,
>
> as announced on IRC, I'm just doing a review, since I'm not a DD
> and can't sponsor:
>
> - packaging in a VCS would be nice to have (plus the appropriate
> Vcs-Browser / Vcs-... headers in d/control)
>
> - debian/copyright:
>
> * Tobias Klauser wasn't just active in 2016, the earliest
> copyright notice of his I could find in the package is
> from 2014; so s/2016/2014-2016/ there
>
> * missing mention of Copyright (C) 2012 Christoph Jaeger
> for pkt.h
>
> * missing mention of Copyright (C) 2009-2012 Daniel
> Borkmann for util.[ch]
The debian/copyright issue is the only major reason I seen not to
upload right now, because this issue will possibly mean rejection
from NEW queue. Please carefully look at all source files
and document copyright holders (autogenerated build files can
be excluded).
>
> - debian/compat: why only 9? compat 10 is considered stable now
> and unless you have a good reason I would recommend that any new
> package should use compat 10. (please read the debhelper manual
> though for information on what changed between 9 and 10)
(compat 10 also gives you a nice automatic dh-autoreconf and
dh-systemd. Don't forget both to bump debian/compat and the
debhelper build-dependency.)
>
> - init.d: this file name works with dh_installinit, but is not
> documented, so I'd recommend using llmnrd.init as the file name
I see you're already credited by upstream so I assume you have
already established a good relationship with your upstream.
That's very good and very useful. Keep your upstream happy.
Upstreams like contributions. You have a golden opportunity
on upstream issue #2.
>
> - init.d: any particular reason you don't use init-d-script? (See
> current /etc/init.d/skeleton for how this works; it will
> automatically source /etc/default/$scriptname and interpret the
> DAEMON_ARGS variable, so your init script could probably be just
> a couple of lines that set the name of the executable)
I'd recommend *against* "init-d-script". It has several outstanding
issues, is unmaintained/orphaned/unproven and AIUI that also means the
init script becomes debian-only.
>
> - any reason you don't install the systemd service provided by
> upstream in addition to the init script?
Please do. Please also consider improving the systemd service
shipped by upstream. (Another golden opportunity for upstream
contributions.)
Most importantly have a look at the User= directive as it seems
like running unprivilegied is preferred (see upstream issue #4).
See also the Restrict*= directives provided by systemd which
would also be nice to limit the potential attack surface.
>
> - debian/rules: nice and clean, I like it
>
> - upstream's build system does git id to get the git revision of
> the current source - but that will clash if you have the packaging
> in git (which can happen implicitly when someone checks out the
> package source via e.g. dgit)
>
> Minor cosmetic thing, but makes the package non-reproducible
> depending on whether you build from unpacked .dsc or from a git
> environment
>
> - lintian warnings:
> W: llmnrd: binary-without-manpage usr/bin/llmnr-query
> W: llmnrd: binary-without-manpage usr/sbin/llmnrd
>
>
> - you should probably add a line "export Q =" to debian/rules to
> disable silent builds. While these look nicer, automated build
> log scanners such as blhc aren't able to catch problems.
debhelper today automatically disables silent rules when building
on buildds. Using Q environment variables isn't the normal thing though.
Even better than to explicitly disable silent build would be to
hook up Q to the automatic debhelper version (V=1?).
>
> - Building in sbuild appears to work fine.
>
> - Package appears to work fine (though I don't have any llmnr
> device running at the moment, so I could only test name
> resolution of my own system)
>
> Regards,
> Christian
>
Regards,
Andreas Henriksson
Reply to: