[pkg] CurveDNS - review
> Hi St?phane
Hi Lucas !
,
>
> On Tue, 20 Jun 2017 13:58:20 +0200
> St?phane Neveu <stefneveu at gmail.com> wrote:
>> May I ask you to review my first package : CurveDNS ?
>> It's on alioth...
>
> Thanks for your work! I had a look at your package. Disclaimer: I'm a
> fairly new contributor myself, doing a review mostly to learn something
> myself and hopefully take some load off the DDs.
>
Thank you for reviewing my package :)
>
> I noticed the source includes (and compiles+uses) the nacl library.
> While that library is already packaged in Debian, it's unfortunately
> not available as shared object.
>
> I don't really know how to proceed here, none of our options look
> particularly promising:
> * Keep the copy of the nacl library: this means that in case of security
> fixes in the nacl library, these need to be applied to your package
> separately.
> * Build-Depend on libnacl-dev and use the provided `libnacl.a` for
> linking: This means that after a security update your package "only"
> needs to be recompiled. But an incompatible ABI change in libnacl
> might break your package without warning?
> * Convince the maintainer of nacl to provide it as shared library
> (upstream does not support this, so this may involve a lot of work and
> may not be doable): This would be what we want, security patches can
> affect you directly and ABI compatibility can be handled somewhat
> gracefully.
>
> I hope someone here can provide us with guidance what to do.
>
Yes, I don't know how to deal with that really. Should I ask on
mentors mailing list ?
I'm not sure to be able to convince the maintainer.
>
> Some possible problems I've noticed when going through the files:
> * To what extend does the software need daemontools? Can't systemd or
> sysvinit run it also? This part creates a lot of complexity (also in
> the maintainer scripts) and I would like to know if we really need
> this.
In fact, the whole documentation on their website is based on
daemontools but ok I'll try to use systemd / sysVinit instead :)
> I also don't understand why we need to start multilog in ExecStartPost
> in the systemd unit file (not present in the init script). Can't we
> let curvedns log to journal? Then we can also drop the whole
> envuidgid stuff and select the correct user in the unit file directly.
> * I think with your hardening_support.patch, only the CPPFLAGS from
> `dpkg-buildflags` are applied and not the CFLAGS or LDFLAGS?
Meaculpa, I'll change that, thanks.
> * debian/curvedns.config: don't you need to db_get in order to be able
> to use $RET (it's commented out)?
Suprisingly, it does not work with db_get whereas it does when
commented out... did I missed something ? yes probably.
> * debian/curvedns.{postrm,prerm}: you should not do something like
>
> if not_my_phase; then
> exit 0
> fi
> #commands here
>
> because that will prevent #DEBHELPER# later to execute what it may
> need to in other phases. Instead do something like:
>
> if my_phase; then
> #commands here
> fi
>
> * Suggestions for "beautification":
> - debian/control: make whitespace consistent, don't mix tabs and
> spaces
Thank you, it's done.
> - debian/copyright: link to format could be https :)
Well the tls certificate doesn't seem to be valid :/
https://curvedns.on2it.net/
> - debian/{dirs,install,manpages}: rename to debian/curvedns.{...} for
> consistency
> - permissions in the repository for the maintainer scripts are
> inconsistent (only some of them have execute permission)
>
Thank you, done :)
Stephane
Reply to: