Bruce Rothermal wrote: > Made changes from review > > http://cr.opensolaris.org/~bruce_r/powerman-2.3/ > > Any other issues
Maybe it's just late or this has been covered while I was having my email issues earlier, but in usr/src/cmd/Makefile you seem to be removing sg3_utils and konkretcmpi along with the "konkretcmpi: cimserver" dependency. Same with usr/src/pkgdefs/Makefile where you are removing SUNWkonkretcmpi and SUNWsg3utilsr/SUNWsg3utilsu You also need to run 'wx ea' and 'wx redelget' to collapse the revisions and synchronise the comments. The single comment should begin with the CR number you are using for the integration followed by the CR synopsis. See http://cr.opensolaris.org/~lxin/iperf/ for an example. Amanda > > 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 > > _______________________________________________ > sfwnv-discuss mailing list > sfwnv-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
