Re: new package: mail/akpop3d

[tmp%bh0.amt.ru@localhost]

On 04/30/20 03:20, Roland Illig wrote:
> On 29.04.2020 17:22, tmp%bh0.amt.ru@localhost wrote:
>> Hello.
>>
>> I ported akpop3d from OpenBSD ports collection
>> (https://cvsweb.openbsd.org/ports/mail/akpop3d/).
>>
>> The package builds and runs fine on my NetBSD 8.2 ((GENERIC) #0: Tue Mar
>> 31 05:08:40 UTC 2020
>> mkrepro%mkrepro.NetBSD.org@localhost:/usr/src/sys/arch/i386/compile/GENERIC i386)
>> with up to date packages.
>>
>> Could you please review and commit it to pkgsrc if found good?
>
> Here are some comments that pkglint won't give you right now:
>
> files/akpop3d.sh looks fine.
>
> patches/patch-akpop3d_8 has a hard-coded /etc. Pkglint already throws an
> error at you if you hard-code /usr/pkg, but it doesn't yet detect /etc
> and /var.  To fix this, add a SUBST block to the package Makefile.  See
> chat/kgb-bot/Makefile for an example, or mk/subst.mk for the
> documentation.  To see pkglint's explanation, you can temporarily add
> the string /usr/pkg to the patch and run pkglint on it.
>
> patches/patch-akpop3d_8: Since the manual page has been modified, the
> date in line 3 should be updated.
>
> patches/patch-authenticate.c contains "MAX_TEMPSTR 100 /* obvious */".
> That's not correct. It is far from obvious why the perfect maximum size
> of the temporary string is exactly 100. Why not 128, or 97?
>
> patches/patch-authenticate.c contains lots of newly written code.  In
> that code, there should be no parentheses around the "return" value.
> That patch should also document whether it is intended to be submitted
> upstream, or if not, why not.  And if it is intended for upstream, it
> should follow the indentation style of the remaining files from that
> project.
>
> patches/patch-authenticate.c contains %sysconfdir% at the bottom. Where
> is that placeholder actually replaced? The package Makefile doesn't lose
> a word about this topic, so I guess it's in the package's upstream
> Makefile. Why are there two placeholders, SYSCONFDIR and %sysconfdir%?
>
> patches/patch-lock_maildrop_c: why the umask change? This must be
> documented.
>
> patches/patch-main_c: the #ifndef at the top is not needed since this
> macro will always be defined by the pkgsrc package Makefile.
>
> patches/patch-main_c: why did the timeout change? Who is going to wait
> 10 minutes for a mail client? I am not that patient.
>
> patches/patch-main_c: in "int c,i", there is a space missing after the
> comma.
>
> patches/patch-main_c: in "maxfd = -1, i=0", spacing must be made
> consistent. Either both assignments must get a space around the "=" or
> none of them.
>
> patches/patch-pop3_session_c: it's strange that the argument to
> write_line must include the trailing "\r\n". That's already encoded in
> the function name. But that's upstream's business, not yours.
>
> patches/patch-tcp_listen_c: inconsistent spacing after comma: LISTENQ,
> deffds.
>
> Makefile: When a package gets initially imported, PKGREVISION must not
> be set. It does not matter what the PKGREVISION was in the OpenBSD ports
> collection, counting starts anew in pkgsrc.
>
> Makefile: The HOMEPAGE should use https instead of http since we're not
> in the 1990s anymore.
>
> Makefile: PKGSRC_COMPILER must not be set by a package since it is a
> user-settable variable. See mk/compiler.mk. Just remove that line.
>
> Makefile: Only one CONFIGURE_ARGS per line, please. (This is something
> that pkglint _could_ also remark, but it's not implemented yet.) This is
> to prevent overlooking some of the configure options.
>
> Makefile: LIBS=-lc is rarely necessary. Especially not _before_ the
> other libraries.
>
> Makefile: Before any ?= assignments of user-settable variables, .include
> "../../mk/bsd.prefs.mk".
>
> Makefile: In do-install, it's inconsistent that there is a trailing
> slash after man8/, but not after sbin.
>
> Other than all these remarks, the package looks fine.
>
> Since the program is so old, be sure to compile it with "gcc -Wall
> -Wextra -O2" once, to see whether it has obvious bugs.
>
> Roland

Roland, thank you for your analysis. I'll try to fix all these issues.


--
Alexei