Pavel, See below for my comments ...
Paul Pavel Tatashin wrote: > I would like to ask you to review mrxvt port: > > http://cr.opensolaris.org/~soleen/webrevs/mrxvt_01/ > > No changes to mrxvt package at all, I added argument "-xc99=all,no_lib" > because the default -xc99=all for some reason makes mrxvt not to > update tput cols/tput lines values. > I also had to manually convert manual page, because it was in -mdoc format. === Start of Comments ==== 1. usr/src/cmd/mrxvt/METADATA Add lines for .. URL: <link to source code location> SUPPORT: ???? BUGTRAQ: solaris/utility/???? 2. usr/src/cmd/mrxvt/Makefile.sfw Use the predefined value for '--prefix=..' from Makefile.master; or even better still use the stuff from CONFIGURE_OPTIONS, eg ... ./configure $(CONFIGURE_OPTIONS) see other recent integrations for examples, eg cups. You might want to apply 'Roland Mainz' comments ... - Use "env - ..." and not "env ..." in the Makefiles to make sure "configure" & "make" only see the env variables they should really get(and not pick-up any random env variable) - Use either $(SHELL) or /usr/bin/bash for "configure" calls (so we know which one is used and "configure" doesn't pick one itself). You could extract the version info from the METADATA file as per 'Christopher Mi' comment .. - Use the method define in Makefile.master since you have a standard METADATA file, eg. .. VER =$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) TARBALL =$(VER).tar.bz2 3. usr/src/cmd/mrxvt/install-sfw You might want to apply 'Roland Mainz' comments ... - use /usr/bin/ksh93 or /usr/bin/bash for install-* and add a $ set -o errexit # at the beginning and replace ".${SRC}/tools/install.subr" with "source ${SRC}/tools/install.subr" (the idea is to catch failures in the script and abort it at that point, right now the script will just continue). I don't think you need the '_install M ..' for your man page is it already includes the sun stability stuff; so you can just use '_install N ..' I think (but check). 4. usr/src/cmd/mrxvt/mrxvt.1.sunman Does this need a sun copyright statement in it? 5. usr/src/pkgdefs/SUNWmrxvt/depend This looks like the default 'depend' file; if you have no other dependencies for this package then ... - remove this file - add 'DATAFILES= depend' to SUNWmrxvt/Makefile But have you checked the dependencies using the dependency checker script(s) to ensure you have no others. I think Alan Coopersmith could probably point you too those scripts. If you keep this file, move the Copyright lines to after the 'CDDL HEADER END'. 6. usr/src/pkgdefs/SUNWmrxvt/pkginfo.tmpl You don't need the version on the NAME= line. 7. usr/src/pkgdefs/SUNWmrxvt/prototype_com & usr/src/cmd/mrxvt/install-sfw Do all these files (f) need the 'write' permission bit set? Switch it off if not in /usr. 8. usr/src/pkgdefs/SUNWmrxvt/prototype_sparc & usr/src/pkgdefs/SUNWmrxvt/prototype_i386 Not that is matters (being picky), but these normally have the pkg name on a comment line at the bottom. === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
