Frank, Below are my comments, some of them might duplicate those from Nils, ...
Paul Frank Jennings wrote: > Requesting a code review of doxygen (Source Code Documentation Tool) for > integration into SFW consolidation. > > Doxygen 1.5.7.1 (stable) was released couple of weeks back. So sending > the updated webrev for review. > > The webrev is at http://cr.opensolaris.org/~frankj/doxygen2 === Start of Comments === 1. sccs idents on new files It looks as though you need to correct the sccs ident lines - to me they look like the ones from the files you used as templates, eg. the "%Z%%M% %I%" stuff 2. usr/src/cmd/doxygen/Makefile.sfw You might want to extract the VER= from the METADATA info, eg. VER =$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) TARBALL =$(VER).tar.gz Are you sure the 'all:' make and the 'config.status:' configure will not pick up environment variables you don't really want them to, ie. you aren't using "env - ..." You might want to explicitly define 'prefix=..' for your configure options using the default value set in Makefile.master (so you would get any global change). Shorten the 'DOXYGEN_CONFIGURE_OPTIONS =' line, eg .. DOXYGEN_CONFIGURE_OPTIONS = --platform solaris-cc DOXYGEN_CONFIGURE_OPTIONS += --enable-langs nl,.... 3. usr/src/cmd/doxygen/install-doxygen Not that is matters but this would commonly be called install-sfw. Remove the write permission on doxygen & doxytag You might want to apply these comments ... Roland Mainz wrote: > Use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* > 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) 4. usr/src/pkgdefs/SUNWdoxygen/Makefile Not that it really matter but normally the DATAFILES= line would come before the 'all:' line. 5. usr/src/pkgdefs/SUNWdoxygen/pkginfo.tmpl Its more normal to put the name on the NAME= line, eg. NAME="Doxygen - Source Code Documentation Tool" Some people put the version at the end of the DESC= line, eg. DESC="............... (1.5.7.1)" 6. usr/src/cmd/doxygen/doxygen_manual-1.5.7.1.pdf Is this the standard manual file? Does it come as part of the source tarball - if it does why not get it from there? 7. depend You are using the default dependencies file, have you checked that your package has not other dependencies with the package dependency checker script? === End of Comments ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
