Bug 225924

Comment 1 Tobias Kortkamp 2018-05-10 15:30:16 UTC

Hi,

thanks for your submission. Unfortunately it's not in a committable
state right now.  Some comments below.

In general the order of variables is wrong. Please see [1] for how to
order them.

XPORTNAME=	kadnode
XPORTVERSION=	2.2.0
XPORTREVISION=	1

We start port revisions at zero, so either remove PORTREVISION or set it
to 0.

XCATEGORIES=	dns
XDISTDIR=	${PWD}

This doesn't make sense. Please do not set DISTDIR. It's not a variable
that ports should overwrite. The ports tree could be mounted read-only
and then the build would fail. It's always mounted read-only in
Poudriere jails which is what we use to build the packages on pkg.FreeBSD.org.

XMAINTAINER=	moritzwarning@web.de
XCOMMENT=	P2P name resolution daemon
XLICENSE=	MIT

Add LICENSE_FILE if there is one in the tarball.

XUSES=		gmake

XWRKSRC=		${WRKDIR}/KadNode-${PORTVERSION}
XMASTER_SITES=   https://github.com/mwarning/KadNode/archive/
XDISTNAME=	v${PORTVERSION}

Please look into USE_GITHUB in the porter's handbook.

XCATEGORIES=	dns

Already set above.

XOPTIONS_DEFINE=	AUTH CMD DEBUG DNS LPD NATPMP NSS UPNP
XAUTH_DESC=	Authorization support based on mbedtls.
XCMD_DESC=	Command line control tool kadnode-ctl.
XDEBUG_DESC=	Build with debug messages and symbols.

DEBUG already has a description in bsd.options.desc.mk so please remove
this one.

XDNS_DESC=	Include local DNS interface.
XLPD_DESC=	Local peer discovery.
XNATPMP_DESC=	NAT-PMP support. Enables remote port forwarding on the router.
XNSS_DESC=	Name Service Switch support to intercept host queries (/etc/nsswitch.conf).
XUPNP_DESC=	UPnP support. Enable remote port forwarding on the router.
X
XOPTIONS_DEFAULT=	AUTH CMD LPD NSS
X
X.include <bsd.port.options.mk>
X
X.if ${PORT_OPTIONS:MAUTH}
XFEATURES+=bob tls
XDEPENDS+=	${PORTSDIR}/security/mbedtls
X.endif

Please look into options helpers. Also I'm not sure what DEPENDS does?

X
X.if ${PORT_OPTIONS:MCMD}
XFEATURES+=cmd
X.endif
X
X.if ${PORT_OPTIONS:MDEBUG}
XFEATURES+=debug
X.endif
X
X.if ${PORT_OPTIONS:MDNS}
XFEATURES+=dns
X.endif
X
X.if ${PORT_OPTIONS:MLPD}
XFEATURES+=lpd
X.endif
X
X.if ${PORT_OPTIONS:MNATPMP}
XFEATURES+=natpmp
XLIB_DEPENDS+=	libnatpmp.so:${PORTSDIR}/net/libnatpmp
X.endif

Please remove the ${PORTSDIR}/ prefix from dependency specs. It hasn't
been required in a long time now.

If you set DEVELOPER=yes in /etc/make.conf during port development then
you'll get warnings about things like this.

X
X.if ${PORT_OPTIONS:MNSS}
XFEATURES+=nss
X.endif
X
X.if ${PORT_OPTIONS:MUPNP}
XFEATURES+=upnp
XLIB_DEPENDS+=	libminiupnpc.so:${PORTSDIR}/net/miniupnpc
X.endif
X
XMAKE_ENV+=	FEATURES="${FEATURES}"
X
Xdo-install:
X	${MKDIR} ${STAGEDIR}/usr/bin
X	${INSTALL_PROGRAM} ${WRKSRC}/build/kadnode ${STAGEDIR}${PREFIX}/bin/
X	${LN} -sf kadnode ${STAGEDIR}${PREFIX}/bin/kadnode-ctl
X
X	${INSTALL_LIB} ${WRKSRC}/build/libnss_kadnode.so.2 ${STAGEDIR}${PREFIX}/lib/nss_kadnode.so
X	${LN} -sf ${STAGEDIR}${PREFIX}/lib/nss_kadnode.so ${STAGEDIR}${PREFIX}/lib/nss_kadnode.so.1

Use ${RLN} instead. This points into the non-existing STAGEDIR (after
make clean) otherwise.

X
X	${MKDIR} ${STAGEDIR}/etc/kadnode
X	${INSTALL_DATA} ${WRKSRC}/misc/peers.txt ${STAGEDIR}/etc/kadnode/
X	${INSTALL_DATA} ${WRKSRC}/misc/kadnode.conf ${STAGEDIR}/etc/kadnode/

The config files need to go into ${STAGEDIR}${ETCDIR}. Putting files
into /etc from the base system is off-limits for ports.

X	${MKDIR} ${STAGEDIR}/etc/rc.d/
X	${INSTALL_SCRIPT} ${WRKSRC}/freebsd/files/kadnode.init ${STAGEDIR}/etc/rc.d/kadnode

rc.d scripts needs to go into ${STAGEDIR}${PREFIX}/etc/rc.d.  Look into
USE_RC_SUBR for this.

echo x - kadnode/pkg-descr
sed 's/^X//' >kadnode/pkg-descr << '3923b8a530675a9c5ca9607acba3d1c2'
XKadNode is a small decentralized DNS resolver that can use existing public key infrastructures.
XIt utilizes the BitTorrent P2P network and mbedtls for TLS/crypto support.
X
XWWW: https://github.com/mwarning/KadNode

Please wrap this to 72 columns.

3923b8a530675a9c5ca9607acba3d1c2
echo x - kadnode/pkg-plist
sed 's/^X//' >kadnode/pkg-plist << 'ee61f23e7c6fbbf7d4eed6f11c8b046c'
Xbin/kadnode
Xbin/kadnode-ctl
Xlib/nss_kadnode.so
Xlib/nss_kadnode.so.1
Xman/man1/kadnode.1.gz
X/etc/rc.d/kadnode
X/etc/kadnode/kadnode.conf
X/etc/kadnode/peers.txt
X@dir /etc/kadnode
ee61f23e7c6fbbf7d4eed6f11c8b046c
echo c - kadnode/files
mkdir -p kadnode/files > /dev/null 2>&1
echo x - kadnode/files/kadnode.init
sed 's/^X//' >kadnode/files/kadnode.init << '0ee9ae9bd1c603ce26dc9c2f7241d306'
X#!/bin/sh
X
X# PROVIDE: kadnode
X# REQUIRE: SERVERS
X# BEFORE: DAEMON
X# KEYWORD: shutdown
X
X. /etc/rc.subr
X
Xname=kadnode
Xrcvar=kadnode_enable
X
Xpidfile="/var/run/${name}.pid"
X
Xcommand="/usr/local/bin/${name}"
Xkadnode_flags="--config /etc/kadnode/kadnode.conf --pidfile $pidfile --daemon"
X
Xrequired_files="/etc/kadnode/${name}.conf"
X
X#add/remove kadnode from /etc/nsswitch.conf
Xstart_precmd="kadnode_precmd"
Xstop_postcmd="kadnode_postcmd"
X
Xkadnode_precmd() {
X	sed -i -e '/kadnode/!s/^\(hosts:.*\)dns\(.*\)/\1kadnode dns\2/' /etc/nsswitch.conf
X}
X
Xkadnode_postcmd() {
X	sed -i -e 's/^\(hosts:.*\)kadnode \(.*\)/\1\2/' /etc/nsswitch.conf
X}

This is not good. rc.d scripts should not modify system files. Leave it
for the user to setup. Maybe write a pkg-message to explain how.

[1] https://www.freebsd.org/doc/en/books/porters-handbook/porting-order.html

Thank your for taking the time to review the port. I will soon address the issues.

Created attachment 193468 [details]
ports files v2

Created attachment 193469 [details]
ports files v3

So... files go into /usr/local/etc/rc.d/kadnode, /usr/local/etc/kadnode/kadnode.conf and /usr/local/etc/kadnode/kadnode.conf?

Comment 6 Tobias Kortkamp 2018-05-17 06:37:55 UTC

(In reply to moritzwarning from comment #5)
Yes, everything the port installs should go under ${PREFIX} (which defaults to
/usr/local but users can set it to something else).

It already looks much better and I think it's close to being ok to commit.

XOPTIONS_DEFAULT= AUTH CMD LPD NSS

Move this just below OPTIONS_DEFINE.

X.if ${PORT_OPTIONS:MAUTH}
XFEATURES+=bob tls
XLIB_DEPENDS+=	libmbedtls.so:security/mbedtls
X.endif
X

As I said above, use options helpers instead.  This is described in detail in
the porter's handbook:
https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/makefile-options.html

For example the above should become

AUTH_LIB_DEPENDS=	libmbedtls.so:security/mbedtls
AUTH_VARS=	FEATURES+="bob tls"

XGH_TAGNAME=	v${PORTVERSION}

This works, but is not optimal.  Remove this and simply set

DISTVERSIONPREFIX=v

See https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/makefile-distfiles.html#makefile-master_sites-github-ex3

Please remove the ===== header and footer from pkg-descr.  I know that a lot
of ports add them but it doesn't really need them and it just adds to the
noise of `pkg install`.

Comment 7 Tobias Kortkamp 2018-05-17 08:31:35 UTC

X	${INSTALL_DATA} ${WRKSRC}/misc/peers.txt ${STAGEDIR}${ETCDIR}
X	${INSTALL_DATA} ${WRKSRC}/misc/kadnode.conf ${STAGEDIR}${ETCDIR}

If users are expected to edit these files then try to install them as sample
files and annotate them in pkg-plist with @sample.

${INSTALL_DATA} ${WRKSRC}/misc/peers.txt ${STAGEDIR}${ETCDIR}/peers.txt.sample
${INSTALL_DATA} ${WRKSRC}/misc/kadnode.conf ${STAGEDIR}${ETCDIR}/kadnode.conf.sample

and in pkg-plist

@sample %%ETCDIR%%/kadnode.conf.sample
@sample %%ETCDIR%%/peers.txt.sample

That way they won't be overwritten on updates.  In case kadnode.conf doesn't exist
yet on a users system it'll be created from kadnode.conf.sample automatically
when the package is installed.

Created attachment 193490 [details]
ports files v4

not working

I tried to fix the remaining issues, but ran into problems.

- the FEATURES variable is not passed to the Makefile
- do I have to use %%MANPREFIX%% in pkg-plist?

Created attachment 193491 [details]
ports files v5

Comment 11 Tobias Kortkamp 2018-05-17 17:51:25 UTC

(In reply to moritzwarning from comment #9)

> the FEATURES variable is not passed to the Makefile

Options helpers need to be before

.include <bsd.port.options.mk>

as that's the point where they're being processed by the framework.
You're basically defining them too late.

But here you don't need

.include <bsd.port.options.mk>

at all, so if you remove that line, it should work.

> - do I have to use %%MANPREFIX%% in pkg-plist?

No. You also don't need %%PREFIX%% in pkg-plist.  Everything in it that
doesn't start with a / is already relative to %%PREFIX%%.

Created attachment 193497 [details]
ports files v6

Let's see if I got it right this time. :>

Comment 14 Tobias Kortkamp 2018-05-18 00:16:09 UTC

Created attachment 193503 [details]
kadnode.diff

This looks ok from the ports side now.

However Kadnode doesn't build when enabling one of the port forwarding
options (NATPMP or UPNP):

src/main.c:66:5: error: invalid operands to binary expression ('int' and 'void')
        rc |= fwd_setup();
        ~~ ^  ~~~~~~~~~~~
1 error generated.

Comment 15 Tobias Kortkamp 2018-05-18 00:21:02 UTC

Created attachment 193504 [details]
kadnode.diff

I will do a minor release shortly that will fix this issue.

How do I get the content of the diff?

Applying it over the current kadnode package folder (patch -p0 < kadnode.diff) creates a lot of .orig files.
Same as for an empty folder as target.

ok, I worked it out. Thank you for fixing all the small issues.

Created attachment 193519 [details]
kadnode port

Comment 20 commit-hook 2018-05-18 20:09:34 UTC

A commit references this bug:

Author: tobik
Date: Fri May 18 20:09:22 UTC 2018
New revision: 470325
URL: https://svnweb.freebsd.org/changeset/ports/470325

Log:
  New port: dns/kadnode

  KadNode is a small decentralized DNS resolver that can use existing
  public key infrastructures. It utilizes the BitTorrent P2P network
  and mbedtls for TLS/crypto support.

  WWW: https://github.com/mwarning/KadNode

  PR:		225924
  Submitted by:	moritzwarning@web.de

Changes:
  head/dns/Makefile
  head/dns/kadnode/
  head/dns/kadnode/Makefile
  head/dns/kadnode/distinfo
  head/dns/kadnode/files/
  head/dns/kadnode/files/kadnode.conf.in
  head/dns/kadnode/files/kadnode.in
  head/dns/kadnode/pkg-descr
  head/dns/kadnode/pkg-message
  head/dns/kadnode/pkg-plist

Comment 21 Tobias Kortkamp 2018-05-18 20:10:09 UTC

Looked good now. Committed. Thanks! :-)

Thanks for helping a newbie!