Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 31 Mar 2015 14:40:34 +0000
From:      Alexey Dokuchaev <danfe@FreeBSD.org>
To:        Renato Botelho <garga@FreeBSD.org>
Cc:        svn-ports-head@freebsd.org, svn-ports-all@freebsd.org, ports-committers@freebsd.org
Subject:   Re: svn commit: r382819 - in head/www: . e2guardian
Message-ID:  <20150331144034.GA31230@FreeBSD.org>
In-Reply-To: <201503311413.t2VEDesm014206@svn.freebsd.org>
References:  <201503311413.t2VEDesm014206@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Mar 31, 2015 at 02:13:40PM +0000, Renato Botelho wrote:
> New Revision: 382819
> URL: https://svnweb.freebsd.org/changeset/ports/382819
> 
> [...]
>   Submitted by:	Marcello Coutinho <marcellocoutinho@gmail.com>
> 
> @@ -0,0 +1,99 @@
> +# Created by: Marcello Coutinho

Would be nice to include creator's email in header as well, at least for
consistency with majority of existing ports.

> +MASTER_SITES=	GH

This line is redundant; in USE_GITHUB case, it will be added by bsd.sites.mk
automatically (see the code around line 533).

> +USE_GITHUB=	yes
> +GH_TAGNAME=	${PORTVERSION:S/^/v/}

Why in-var substitution instead of simple and more readable v${PORTVERSION}?

> +USES=		pkgconfig iconv

Mirordered USES (minor nit).

> +HAS_CONFIGURE=	yes

HAS_CONFIGURE for GNU autoconf-based build system?  Doesn't look right.

> +USE_AUTOTOOLS=	aclocal libtoolize autoheader automake autoconf
> +ACLOCAL_ARGS=	-I m4
> +AUTOMAKE_ARGS=	--add-missing --copy --foreign

Did you consider using USES=autoreconf instead of USE_AUTOTOOLS et al.?

> +OPTIONS_DEFAULT=TRICKLE DOCS 1024

DOCS should not be part of OPTIONS_DEFAULT.

> +APACHE_DESC=	Enable Apache support for access denied page
> +TRICKLE_DESC=	Enable the trickle download manager
> +CLISCAN_DESC=	Enable support for CLI content scanners
> +CLAMD_DESC=	Enable ClamD AV content scanner
> +ICAP_DESC=	Enable ICAP AV content scanner support
> +KAV_DESC=	Enable Kaspersky AV support
> +NTLM_DESC=	Include NTLM authentication plugin
> +DNS_DESC=	Include DNS authetication plugin
> +EMAIL_DESC=	Enable e-mail reporting support

We normally do not prepend the word "Enable" to option descriptions.
See file Mk/bsd.options.desc.mk for examples of well-named options.

> +OPTIONS_RADIO=	DESCRIPTORS
> +OPTIONS_RADIO_DESCRIPTORS=	1024 2048 4096 8192

Description for DESCRIPTORS (DESCRIPTORS_DESC) is missing.

> +pre-configure:

In-place patching is more naturally done in `post-patch' target.

> +	@${REINPLACE_CMD} -e 's|.lresolv||g' \
> +		${WRKSRC}/configure.ac

I'm puzzled over this 's|.lresolv||g' part.  This is the resulting diff I
see:

@@ -788,7 +788,7 @@
        AC_MSG_RESULT(yes)
        dnsauth=true
        DNSAUTHSUPPORT=""
-       CXXFLAGS="${CXXFLAGS} -lresolv"
+       CXXFLAGS="${CXXFLAGS} "
        AC_DEFINE([PRT_DNSAUTH],[],[Define to enable DNS auth plugin])
 fi],
 [

Can you elaborate on why 's|.lresolv||g' was used to achieve this?  I also
do not see the need for global modifier (g).  Notice bogus trailing space
before closing quote.

./danfe



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150331144034.GA31230>