Date: Wed, 4 Sep 2013 03:27:35 +0000 From: Alexey Dokuchaev <danfe@FreeBSD.org> To: Mark Felder <feld@FreeBSD.org> Cc: svn-ports-head@freebsd.org, svn-ports-all@freebsd.org, ports-committers@freebsd.org Subject: Re: svn commit: r326240 - in head/sysutils/monit: . files Message-ID: <20130904032735.GB71557@FreeBSD.org> In-Reply-To: <201309040115.r841FjYT062091@svn.freebsd.org> References: <201309040115.r841FjYT062091@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Sep 04, 2013 at 01:15:45AM +0000, Mark Felder wrote: > New Revision: 326240 > URL: http://svnweb.freebsd.org/changeset/ports/326240 > > [...] > > -MAN1= monit.1 > +MAN1= monit.1 It was indented correctly, why move it one stop closer? Now knob values do not align. Sometimes, esp. for long MANx lists one-stop indentation is OK, as it increases line capacity, which pays off for many lines (like, around ten or something), but for single or couple lines MANx, normal indentation should be used. > -USES= bison > +USES= bison Ditto. Not just it pessimizes indentation instead of improving it; it is what usually called "gratuitous change". Gratuitous changes might seem harmless, yet they decrease STN ratio, and, most importantly, make it hard to "svn blame" and perform other archaeology. > USE_GMAKE= yes You could have converted USE_GMAKE to USES while here. > -DOCS= CHANGES COPYING README > -PORTDOCS= ${DOCS:T} > +PORTDOCS= CHANGES COPYING README License files (COPYING) should not be installed as part of PORTDOCS, it's better to use LICENSE knob for these things. > -.if empty(PORT_OPTIONS:MSSL) > +.if ! ${PORT_OPTIONS:MSSL} > CONFIGURE_ARGS+= --without-ssl > .endif We recently got few nice helpers for simple OPTIONS handling. For example, these three lines could be turned into just one: SSL_CONFIGURE_OFF= --without-ssl Or, if configure script supports --with-ssl, you can use SSL_CONFIGURE_WITH= ssl (--with-/--without- would be added automatically). > -.if !defined(NOPORTDOCS) > - ${MKDIR} ${DOCSDIR} > - cd ${WRKSRC} && ${INSTALL} -m 644 ${DOCS} ${DOCSDIR}/ > +.if ${PORT_OPTIONS:MDOCS} > + ${INSTALL} -d ${DOCSDIR} Please use MKDIR instead of INSTALL -d. MKDIR is a much more readable and common. (How often do you see people creating directories with install?) > + cd ${WRKSRC} && ${INSTALL} -m 644 ${PORTDOCS} ${DOCSDIR} Any reason not to use INSTALL_DATA (and drop -m 644)? You could have also avoided cd'ing and make ${INSTALL} $cwd-agnostic by using this simple trick: ${INSTALL_DATA} ${PORTDOCS:S,^,${WRKSRC}/,} ${DOCSDIR} It's quite common around the tree, and it's concise yet perfectly readable. There are other bugs in this ports's Makefile (poorly sorted knobs, lack of padding around @${CAT} ${PKGMESSAGE}) and pkg-descr (whitespace at EOL, badly spelled abbreviations). ./danfe
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130904032735.GB71557>