Re: new package: mail/akpop3d

[Roland Illig <roland.illig%gmx.de@localhost>]

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