Been on vacation sorry for the delay. Responses below.
Changes posted at http://cr.opensolaris.org/~bruce_r/powerman-2.3/ Thanks 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 Added missing items > > > Add the missing ARC number. I'm no sure you > need that line though > Added ARC number > I don't think you need the COMMUNITY: field, its the > same as URL:, see > http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines > Leaving it. > Delete blank last line > Deleted > Is that really the url to the SRC: (source tarball). > It was the download page. I changed it to the actual bzip tar link. > 2. usr/src/cmd/powerman/Makefile.sfw > Change 'env ' to 'env - ' (throughout) Done > > > Extract VER= info from the METADATA, something like .. > VER = $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh) > TARBALL = $(VER).tar.gz > Done > Do you really need line ... > 46 @find . -name core -exec rm -f {} \; > if not delete it. If you do why is it 'core' dumping? No core dumping. I copied the Makefile from another workspace as a template. I'll delete the line. > > > 3. usr/src/cmd/powerman/sunman-stability > Is 'Interface Stability Volatile' correct? > Interface is not controlled by Sun so it should be Volatile. Man pages are provided in the tar ball. I am not providing any changes to these. Should I be? > Where is this applied to the man pages ? > > 4. usr/src/pkgdefs/SUNWpowermanr/Makefile > Delete the null DATAFILES line Done > > > 5. usr/src/pkgdefs/SUNWpowermanr/depend > Does a root package really have these dependencies? > > Looks like you have a dependency loop ... > SUNWpowermanr -> SUNWpowermanu -> SUNWpowermanr > The powerman software has files that install into both usr and etc but they are all needed to be installed on the system. root package will not allow usr files and vice versa. So I have to make them dependent on each other. This shouldn't be a loop because if you install SUNWpowermanr it will indicate that SUNWpowermanu is needed but will not have an issue if it is already installed. Do you know of any issue with this? > 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)" Done > > > 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 Done > > > 7. usr/src/pkgdefs/SUNWpowermanu/Makefile > Delete the null DATAFILES line Done > > > 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? > Where is this dependency checker script > Line 53, align the descriptive bit to the bit above Done > > > 9. usr/src/pkgdefs/SUNWpowermanu/pkginfo.tmpl > See (6) > Done > === End of Comments ====== > -- > ---------------------------------------------------------------------- > Paul Cunningham > Software Engineer > Tadpole Business Unit
