Bruce, See below for my comments ...
Paul Bruce Rothermal wrote: > Could I please get a code review for Powerman-2.3 > > webrev output at: http://cr.opensolaris.org/~bruce_r/powerman-2.3/ === Start of Comments ==== 1. usr/src/cmd/powerman/METADATA Add missing field, see ... http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines Add the missing ARC number. I'm no sure you need that line though I don't think you need the COMMUNITY: field, its the same as URL:, see http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines Delete blank last line Is that really the url to the SRC: (source tarball). 2. usr/src/cmd/powerman/Makefile.sfw Change 'env ' to 'env - ' (throughout) Extract VER= info from the METADATA, something like .. VER = $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) TARBALL = $(VER).tar.gz Do you really need line ... 46 @find . -name core -exec rm -f {} \; if not delete it. If you do why is it 'core' dumping? 3. usr/src/cmd/powerman/sunman-stability Is 'Interface Stability Volatile' correct? Where is this applied to the man pages ? 4. usr/src/pkgdefs/SUNWpowermanr/Makefile Delete the null DATAFILES line 5. usr/src/pkgdefs/SUNWpowermanr/depend Does a root package really have these dependencies? Looks like you have a dependency loop ... SUNWpowermanr -> SUNWpowermanu -> SUNWpowermanr 6. usr/src/pkgdefs/SUNWpowermanr/pkginfo.tmpl You might want to put the version at the end of the DESC= line, eg. ... DESC="...... (6.0)" A root pkg would also normally have "(root)" at the end of both the DESC and NAME lines. Similarly the user package would have "(user)" I think - check 7. usr/src/pkgdefs/SUNWpowermanu/Makefile Delete the null DATAFILES line 8. usr/src/pkgdefs/SUNWpowermanu/depend Have you run the dependency checker script against your package to ensure you have picked up all its dependencies? Line 53, align the descriptive bit to the bit above 9. usr/src/pkgdefs/SUNWpowermanu/pkginfo.tmpl See (6) === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
