svn commit: r318883 - in head/net-im: ejabberd iserverd jabber-pyaim jggtrans naim py-xmpppy-yahoo sim-im-devel skype skype-devel tkabber-devel

[Alexey Dokuchaev]

On Thu, May 23, 2013 at 04:10:43PM +0000, Jason Helfman wrote:
> New Revision: 318883
> URL: http://svnweb.freebsd.org/changeset/ports/318883
> 
> -.if defined(NOPORTDOCS)
> +.if ! ${PORT_OPTIONS:MDOCS}
>  MAKE_ARGS+=	NOPORTDOCS=${NOPORTDOCS}
>  .endif

This is a bit worrisome: it seems that this code still depends on NOPORTDOCS
variable (while this commit supposedly gets rid of it).  Could this become a
problem in the future?

> -OPTIONS=	KQUEUE "Use kqueue(2) instead of poll(2)" on \
> -		DEBUG "Enable debugging symbols" off
> +OPTIONS_DEFINE=	KQUEUE DEBUG
> +KQUEUE_DESC=	kqueue(2) instead of poll(2)

I think you've missed "Use" word here.  Now options description both reads
(missing verb or subject word like "support"; kqueue/poll here are more of
objects in grammatical sense) and looks bad (starts with lowercase letter).

> -OPTIONS=	EJABBERD "Use transport with ejabberd" off \
> -		TWISTED1 "Use old py-twisted 1.x" off
> +OPTIONS_DEFINE=	EJABBERD TWISTED1
> +EJABBERD_DESC=	transport with ejabberd
> +TWISTED1_DESC=	old py-twisted 1.x

Ditto.

> +.if ${PORT_OPTIONS:MDOCS}
>  	${MKDIR} ${DOCSDIR}
>  .for portdoc in ${PORTDOCS}
>  	${INSTALL_DATA} ${WRKSRC}/${portdoc} ${DOCSDIR}/

There is a nice (concise yet readable) way of avoiding .for loop here:

  .if ${PORT_OPTIONS:MDOCS}
	@${MKDIR} ${DOCSDIR}
	${INSTALL_DATA} ${PORTDOCS:S,^,${WRKSRC}/,} ${DOCSDIR}
  .endif

> -OPTIONS=		NODEBUG "Turn off debugging code" off \
> -			EJABBERD "Use transport with ejabberd" off
> +OPTIONS_DEFINE=	DEBUG EJABBERD
> +EJABBERD_DESC=	transport with ejabberd

Ditto.

> -OPTIONS=	DETACH "Enable 'detach' feature (requires misc/screen)" off
> +OPTIONS_DEFINE=	DETACH
> +DETACH_DESC=	Activate 'detach' feature (requires misc/screen)

"Enable" is not a taboo :) and suits just fine here.  It is normally dropped
when used together with "support" as being redundant: "Blah support" already
implies (at least in options dialog context) that it can be enabled (checked)
or disabled (unchecked).

"Activate" is less neutral; it might confuse users (it does confuse me): I
think that activating 'detach' feature is what happens during runtime when
user is actually *detaches*, not when this feature is *enabled* during the
port config/build time.

> -OPTIONS=	EJABBERD "Use transport with ejabberd" off
> +OPTIONS_DEFINE=	EJABBERD
> +EJABBERD_DESC=	transport with ejabberd

This one is a good example of why weak-subject descriptions look bad.  Not
to mention initial lowercase letter...

> +OPTIONS_DEFINE=	VIDEO NVIDIA_GL
> +VIDEO_DESC=[broken] Video support
> +NVIDIA_GL_DESC=	libGL provided by NVidia binary drivers

Ditto; also it seems indentation in VIDEO_DESC= line got broken.

> -OPTIONS=	VIDEO	"Video sending support via multimedia/webcamd"	on \
> -		NVIDIA_GL "Use libGL provided by NVidia binary drivers" off
> +
> +OPTIONS_DEFINE=	VIDEO NVIDIA_GL
> +NVIDIA_GL_DESC=	libGL provided by NVidia binary drivers

Ditto.  They read better with "Use".

./danfe