Made changes from review http://cr.opensolaris.org/~bruce_r/powerman-2.3/
Any other issues Bruce On Feb 10, 2009, at 1:20 AM, Paul Cunningham wrote: > Bruce, > > Bruce Rothermal wrote: >> Please review >> All changes made from first review: >>>> webrev output at: http://cr.opensolaris.org/~bruce_r/powerman-2.3/ > > A few more comments ... > > 1. usr/src/cmd/powerman/METADATA > Delete the "COMMUNITY: " line -= its the same as the "SRC: " > line Deleted > > > Make the 'Name: ' line more descriptive changed to: PowerMan Power Management Software > > > 2. usr/src/cmd/Makefile > & usr/src/pkgdefs/Makefile > It looks as though you need to resync these with the gate, > otherwise it looks as though you are trying to delete stuff Will resync - These to file changes have to be backed out before new sync or they will have conflict problems with bringover. > > > 3. usr/src/cmd/powerman/install-powerman > Rename this to install-sfw, is more normal name Done > > > 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) Changed to bash > > > Why the 'mv ' lines for man pages ?? eg. why ... > 44 _install M $MAN1/powerman.1 $MAN1/powerman.1.temp 444 > 45 mv $MAN1/powerman.1.temp $MAN1/powerman.1 > instead of just ... > _install M $MAN1/powerman.1 $MAN1/powerman.1 444 > Tried without mv and it overwrote the files as I originally thought it would leaving man files empty. -r--r--r-- 1 brucer staff 0 Feb 10 07:45 powerman.1 > 4. Copyright year > Change in all files to 2009 All changed to 2009 > > > 5. usr/src/cmd/powerman/Makefile.sfw > Delete ... > #VER=powerman-2.3 > otherwise you will have to update it for every version > update Deleted > > > 6. usr/src/cmd/powerman/sunman-stability > Aren't the man pages delivered by SUNWpowermanu > rather than SUNWpowermanr Changed sunman-stability to SUNWpowermanu > > > Is 'Interface Stability Volatile' correct? ARC was done with Volatile > > > 7. usr/src/pkgdefs/SUNWpowermanr/copyright > & usr/src/pkgdefs/SUNWpowermanu/prototype_com > Do you need to add originator copyright lines and the > sun disclaimer? I think this is all done now if I understand the issue. > > > Paul >> 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
