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
Make the 'Name: ' line more descriptive
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
3. usr/src/cmd/powerman/install-powerman
Rename this to install-sfw, is more normal name
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)
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
4. Copyright year
Change in all files 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
6. usr/src/cmd/powerman/sunman-stability
Aren't the man pages delivered by SUNWpowermanu
rather than SUNWpowermanr
Is 'Interface Stability Volatile' correct?
7. usr/src/pkgdefs/SUNWpowermanr/copyright
& usr/src/pkgdefs/SUNWpowermanu/prototype_com
Do you need to add originator copyright lines and the
sun disclaimer?
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
