Date: Thu, 14 Mar 2002 08:00:04 -0800 (PST) From: Peter Pentchev <roam@ringlet.net> To: freebsd-ports@FreeBSD.org Subject: Re: ports/35755: Update to sysutils/LPRng port Message-ID: <200203141600.g2EG04M31739@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR ports/35755; it has been noted by GNATS. From: Peter Pentchev <roam@ringlet.net> To: Patrick Powell <papowell@lprng.com> Cc: bug-followup@FreeBSD.org Subject: Re: ports/35755: Update to sysutils/LPRng port Date: Thu, 14 Mar 2002 18:00:01 +0200 On Sun, Mar 10, 2002 at 05:22:42PM -0800, Patrick Powell wrote: > > >Number: 35755 > >Category: ports > >Synopsis: Update to sysutils/LPRng port > >Originator: Patrick Powell > >Description: > update to LPRng port , ran portlint Okay, now. I'd like to ask you to please not take any of the following in a bad way; although it might seem like a rather harsh dissection of your port update patch, it is NOT in any way meant to insult your efforts or your good will in trying to maintain the LPRng port. Try to think of it as simply some advice towards enhancing a port that you have done a good job at updating! All in all, I have to ask you to take note of my comments below and resubmit your port update patch after appropriate modifications. > >Fix: > > > diff -rNu /usr/ports/sysutils/LPRng/Makefile LPRng/Makefile > --- /usr/ports/sysutils/LPRng/Makefile Wed Oct 17 12:14:21 2001 > +++ LPRng/Makefile Sun Mar 10 17:20:21 2002 > @@ -1,50 +1,70 @@ > # New ports collection makefile for: LPRng > -# Date created: 2 Apr 1997 > -# Whom: desmo@bandwidth.org > +# Date created: 10 Oct 2001 > +# Whom: papowell@lprng.com Erm.. I do not think that it is customary to change those lines. They really do reflect the time the port was *created*, not last modified :) > # > -# $FreeBSD: ports/sysutils/LPRng/Makefile,v 1.19 2001/10/17 19:14:21 lioux Exp $ > +# $FreeBSD: ports/sysutils/LPRng/Makefile,v 1.10 1999/09/27 01:50:00 steve Exp $ ..and there is no real reason to try and go back in time :) > # > > -PORTNAME= LPRng > -PORTVERSION= 3.7.6 > -CATEGORIES= sysutils print > +PORTNAME=LPRng > +PORTVERSION=3.8.9 > +CATEGORIES= sysutils print > MASTER_SITES= ftp://ftp.lprng.com/pub/%SUBDIR%/ \ > - ftp://ftp.astart.com/pub/%SUBDIR%/ \ > - ftp://ftp.cise.ufl.edu/pub/mirrors/%SUBDIR%/ \ > - ftp://ftp.cs.umn.edu/pub/%SUBDIR%/ \ > - ftp://ftp.sage-au.org.au/pub/printing/spooler/lprng/LPRng/ \ > - ftp://ftp.informatik.uni-hamburg.de/pub/os/unix/utils/%SUBDIR%/ \ > - ftp://ftp.uni-paderborn.de/pub/unix/printer/%SUBDIR%/ > -MASTER_SITE_SUBDIR= LPRng/LPRng > + ftp://ftp.cise.ufl.edu/pub/mirrors/%SUBDIR%/ \ > + ftp://ftp.cs.umn.edu/pub/%SUBDIR%/ \ > + ftp://ftp.informatik.uni-hamburg.de/pub/os/unix/utils/%SUBDIR%/ \ > + ftp://ftp.uni-paderborn.de/pub/unix/printer/%SUBDIR%/ > +MASTER_SITE_SUBDIR= LPRng/LPRng MASTER_SITE_SUBDIR seems to have some whitespace converted from tabs to spaces.. > EXTRACT_SUFX= .tgz > > -MAINTAINER= desmo@bandwidth.org > +MAINTAINER= papowell@astart.com I may have missed something, but the former maintainer really is okay with this change, right? :) > > -LIB_DEPENDS= gdbm.2:${PORTSDIR}/databases/gdbm > +#RUN_DEPENDS= ifhp:${PORTSDIR}/print/ifhp What exactly is the purpose of this commented-out line? :) If the port should depend on print/ifhp, then make it so - uncomment the line. If there is some condition that this dependency depends on, like an update to print/ifhp to a newer version or something, please state it and uncomment the line. I personally see no reason for a commented-out RUN_DEPENDS line in the final version. > > +.if defined(PREFIX) > + CONFIGURE_ARGS+= --prefix="${PREFIX}" > +.endif > +.if defined(SYSCONFDIR) > + CONFIGURE_ARGS+= --sysconfdir="${SYSCONFDIR}" > +.endif > + > +HAS_CONFIGURE= yes > GNU_CONFIGURE= yes Uhm.. GNU_CONFIGURE implies HAS_CONFIGURE, there is no need to state it explicitly. > -CONFIGURE_ARGS= \ > - --with-sbindir=${PREFIX}/sbin \ > - --with-filterdir=${PREFIX}/libexec/filters \ > - --with-lpd_conf_path=${PREFIX}/etc/lpd.conf \ > - --with-lpd_perms_path=${PREFIX}/etc/lpd.perms \ > - --with-printcap_path=/etc/printcap \ > - --enable-gdbm=${LOCALBASE} > - > -MAN1= cancel.1 lp.1 lpbanner.1 lpf.1 \ > - lpq.1 lpr.1 lprm.1 lpstat.1 monitor.1 \ > - pclbanner.1 psbanner.1 > -MAN5= lpd.conf.5 lpd.perms.5 printcap.5 > -MAN8= checkpc.8 lpc.8 lpd.8 > -MANCOMPRESSED= yes > +INSTALLS_SHLIB= yes > > -DOC_FILES= CHANGES LPRng-HOWTO.* *.jpg LISA98.ppt > +CONFIGURE_ARGS+= \ > + --with-ldopts="-L${LOCALBASE}/lib" \ > + --with-ccopts="-I${LOCALBASE}/include" \ > + --with-filterdir=${PREFIX}/libexec/filters \ > + --with-ld_library_path="${PREFIX}/lib:/lib:/usr/lib:/${LOCALBASE}/lib" \ > + --with-filter_path="${PREFIX}/bin:/bin:/usr/bin:${PREFIX}/sbin:/sbin:/usr/sbin" > + > +MAN1=cancel.1 lp.1 lpbanner.1 lpf.1 lpq.1 lpr.1 lprm.1 lpstat.1 monitor.1 pclbanner.1 psbanner.1 > +MAN5=lpd.conf.5 lpd.perms.5 printcap.5 > +MAN8=checkpc.8 lpc.8 lpd.8 Was there a reason for this whitespace restructuring? In general, tab-delimited MANx lines look better, even if they should be continued on several lines; this is a condition that a lot of ports live with in the name of readability. By the way, CONFIGURE_ARGS+= as opposed to CONFIGURE_ARGS= poses a subtle risk (that I should probably try and document in the Porter's handbook) - if LPRng is built as a dependency of some other port, this other port's build process will leave a CONFIGURE_ARGS variable in the build environment of LPRng. This may lead to, well, unexpected results :) > + > +pre-everything:: > + @${ECHO_MSG} "If you want to replace the default printing system with LPRng, use:" > + @${ECHO_MSG} " make PREFIX=/usr SYSCONFDIR=/etc clean all install" > +# @${ECHO_MSG} "Configuring with '${CONFIGURE_ARGS}'" Again, a commented-out line that serves no real purpose :) 'make -V CONFIGURE_ARGS PREFIX=/usr-or-whatever' works just fine for me :) > + @if [ "${PREFIX}" = "/usr" -a ! -d /usr/man ] ; then \ > + ${ECHO_MSG} "The man pages will be installed in /usr/man." ; \ > + ${ECHO_MSG} "You should make a symbolic link /usr/share/man from /usr/man"; \ > + ${ECHO_MSG} " ln -s /usr/share/man /usr/man"; \ > + ${ECHO_MSG} "If you do not, you will retain the old FreeBSD man pages."; \ > + ${ECHO_MSG} "See the hier(7) man page for details of the FreeBSD file system"; \ > + ${ECHO_MSG} "layout. Configure is not equipped to determine the location of"; \ > + ${ECHO_MSG} 'man pages and defaults to $${PREFIX}/man, which is incorrect for FreeBSD.'; \ > + exit 1; \ > + fi > > post-install: > +.if !defined(NOPORTSDOCS) > @${INSTALL} -d -o ${SHAREOWN} -g ${SHAREGRP} -m 0555 ${DOCSDIR} > -.for file in ${DOC_FILES} > - @${INSTALL_DATA} ${WRKSRC}/HOWTO/${file} ${DOCSDIR} > +.for ext in html pdf ppt ps txt gif jpg png > + for i in `ls ${WRKSRC}/HOWTO |${GREP} "\.${ext}$$"`; \ > + do ${INSTALL_DATA} ${WRKSRC}/HOWTO/$$i ${DOCSDIR}; done I think this might be done better with some combination of ${FIND} | ${XARGS}.. Even in this case, there should be a space before the ${GREP}. > .endfor > - @${CAT} ${PKGMESSAGE} | ${SED} -e "s|@@PREFIX@@|${PREFIX}|g" > +.endif > + @${SED} -e "s!DOCSDIR!${DOCSDIR}/!" ${PKGMESSAGE} Okay, this is an optimization I agree with :) > > .include <bsd.port.mk> > diff -rNu /usr/ports/sysutils/LPRng/README.html LPRng/README.html > --- /usr/ports/sysutils/LPRng/README.html Wed Dec 31 16:00:00 1969 > +++ LPRng/README.html Sun Mar 10 17:20:21 2002 Eeep... no no no no no! README.html is a dynamically generated file, there is absolutely no need to include it in a patch! Yes, it can be easily removed by the committer before actually committing your patch, but this is one more (needless) thing that the committer might have been spared.. > --- /usr/ports/sysutils/LPRng/files/patch-ah Mon Jul 30 11:18:38 2001 > +++ LPRng/files/patch-ah Wed Dec 31 16:00:00 1969 > @@ -1,24 +0,0 @@ [snip] > diff -rNu /usr/ports/sysutils/LPRng/files/patch-ai LPRng/files/patch-ai > --- /usr/ports/sysutils/LPRng/files/patch-ai Mon Jul 30 11:18:38 2001 > +++ LPRng/files/patch-ai Wed Dec 31 16:00:00 1969 > @@ -1,33 +0,0 @@ [snip] > diff -rNu /usr/ports/sysutils/LPRng/files-pkg-message LPRng/files-pkg-message > --- /usr/ports/sysutils/LPRng/files-pkg-message Wed Dec 31 16:00:00 1969 > +++ LPRng/files-pkg-message Sun Mar 10 17:20:21 2002 > @@ -0,0 +1,24 @@ [snip] > diff -rNu /usr/ports/sysutils/LPRng/pkg-deinstall LPRng/pkg-deinstall > --- /usr/ports/sysutils/LPRng/pkg-deinstall Wed Dec 31 16:00:00 1969 > +++ LPRng/pkg-deinstall Sun Mar 10 17:20:21 2002 > @@ -0,0 +1,28 @@ [snip] > diff -rNu /usr/ports/sysutils/LPRng/pkg-install LPRng/pkg-install > --- /usr/ports/sysutils/LPRng/pkg-install Wed Dec 31 16:00:00 1969 > +++ LPRng/pkg-install Sun Mar 10 17:20:21 2002 > @@ -0,0 +1,146 @@ It would be nice of you to explicitly mention which files are no longer needed, or which ones are new - adding or removing a file requires a separate CVS command invocation, and the committer might not realize that this is needed. (Yes, it is obvious from reading the patch itself, but you know how we committers are - sometimes it is easier to be lazy :PPP ) > diff -rNu /usr/ports/sysutils/LPRng/pkg-descr LPRng/pkg-descr > --- /usr/ports/sysutils/LPRng/pkg-descr Mon Jul 30 11:18:38 2001 > +++ LPRng/pkg-descr Sun Mar 10 17:20:21 2002 > @@ -8,5 +8,5 @@ > do not need to run SUID root; greatly enhanced security checks; and a > greatly improved permission and authorization mechanism. > > -Author: Patrick Powell <papowell@lprng.com> > -WWW: http://www.astart.com/lprng/LPRng.html > +WWW: http://www.lprng.com > +FTP: ftp://ftp.lprng.com Please make those fully-qualified and correct URL's; that is, put the slash at the end. Various scripts for automated processing of the Ports Collection depend on those being fully qualified URL's. > diff -rNu /usr/ports/sysutils/LPRng/pkg-plist LPRng/pkg-plist > --- /usr/ports/sysutils/LPRng/pkg-plist Wed Oct 17 12:14:21 2001 > +++ LPRng/pkg-plist Sun Mar 10 17:20:21 2002 > @@ -1,31 +1,18 @@ > -bin/cancel > -bin/lp > +etc/lpd.perms.sample > +etc/lpd.conf.sample > +etc/printcap.sample > +etc/lprng.sh > bin/lpq > -bin/lpr > bin/lprm > +bin/lpr Please try to keep the pkg-plist sorted in lexicographical order. And finally, once again, please to not take any of this badly; it is just an attempt to guide you away from some of the pitfalls that most porters hit sooner or later :) G'luck, Peter -- Peter Pentchev roam@ringlet.net roam@FreeBSD.org PGP key: http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 What would this sentence be like if it weren't self-referential? To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-ports" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200203141600.g2EG04M31739>