Please review All changes made from first review:
>> webrev output at: http://cr.opensolaris.org/~bruce_r/powerman-2.3/ Thanks Bruce On Dec 19, 2008, at 1:33 AM, Paul Cunningham wrote: > 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 > _______________________________________________ > sfwnv-discuss mailing list > sfwnv-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
