[pkg] CurveDNS - review

[Stéphane Neveu]

Hi Lukas,


> 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.
>

Great, nevertheless I added the name of the guy.

>> >> > [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.
>

Removing the binary-indep is ok but when I remove the clean target,
the build fails :/


>>
>> >
>> >> > [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.
>

Great !

>> >
>> > 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.
>

Done I guess. I also completly removed daemontools.
Thanks again Lukas.

> Regards
> Lukas

Regards,
Stephane