[pkg] CurveDNS - review
Hi
On Fri, 23 Jun 2017 21:47:28 +0200
St?phane Neveu <stefneveu at gmail.com> wrote:
> > Great, that sounds very promising indeed! I only looked at it
> > briefly, I'll take a proper look next week (I'm busy on the
> > weekend). I noticed that there is no attribution in any of the
> > patches. If you based your patches on something from FreeBSD, you
> > should add some form of attribution.
> >
>
> Added, tell me if it's clean enough for you or if it needs to be more
> specific.
Good enough.
> >> > [regarding improvement of debian/curvedns.config script]
> >> Yes, I agree with you. I'm still working on it, I'm trying to add
> >> some more controls but for now the db_input high
> >> curvedns/ask_again is now showing up... Still need to work on this
> >> like you said :)
> >
> > Sounds good, let me know when it's ready for review.
> >
>
> It's a bit better I guess, tell what you think about it.
>
> >> Note : I also added db_purge in postrm.
> >
> > You shouldn't need to add that manually. The code to do that should
> > be automatically inserted where you placed the #DEBHELPER#
> > placeholder (by dh_installdebconf). If you want to make sure it's
> > done, extract the control information from your binary package
> > after a build using `dpkg -e` and check the final script.
> >
>
> Ok deleted.
You have some strange entries in debian/rules:
clean:
debconf-updatepo
binary-indep:
dh_installdebconf
both of these need to be deleted! dh_installdebconf will be called
automatically without the need for you calling it.
Running debconf-updatepo on clean seems plain wrong. You're also
taking away the "clean" target entirely from dh by doing it this way.
If you want to add something to the clean target you should do
something like:
override_dh_clean:
# command you want to run before dh_clean
dh_clean
# command you want to run after dh_clean
I'm not sure if debconf-updatepo should be called automatically by
debian/rules at all, but I've never packaged something with debconf
yet, so I will need to read up on that.
>
> >
> >> > [regarding other improvements of
> >> > debian/curvedns.{postinst,postrm,prerm} ]
> >> Is it a bit better ?
> >
> > Yes, better :) .
> >
> > In curvedns.prerem you got the negation wrong: in the `if` statement
> > the "-a" for and needs to become an "-o" for or (mind the De
> > Morgan's laws when negating).
> >
>
> Sorry, I hope it's good now :)
Seems correct.
> >
> > As noted above, I won't be available on the weekend (so don't take
> > my lack of response for a lack of interest). I've only spent little
> > time just now to look at your recent changes; however, I see you
> > have put good work into the packaging!
> >
> > I will make a more thorough review next week (including actually
> > building the package which admittedly I haven't done so far?).
> >
>
> I still need to work on removing daemontools.
Ok.
One other minor thing I noticed as you've played around with `set`
quite a bit in the maintainer scripts: If your first line is
#!/bin/sh -e
then there is no need to follow up on the first line with `set -e` (the
shebang line has -e already). I personally like it better to remove
the `-e` from the first line and then do the `set -e`, which makes it
more obvious and less easy to miss that errexit is set.
I'll make a thorough review next week as promised.
Regards
Lukas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-security-team/attachments/20170624/ab5acc2d/attachment.sig>
Reply to: