Not removing anything just trying to get code reviews for my portion done before more checkins are done. I understand that usr/src/cmd/ Makefile and usr/src/pkgdefs/Makefile will constantly be updated and I will have to resolve this before I can do putback.
Bruce On Feb 10, 2009, at 2:59 PM, Amanda waite wrote: > 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 >
