[sfwnv-discuss] code review request for integrating sg3 utilities to opensolaris

[xiao li - Sun Microsystems - Beijing China]

Hi Paul,
Please see my comments below.

Paul Cunningham wrote:
> Hi Xiao,
>
> Without looking at anyone else's comments, see below for my comments 
> on the updated webrev (mainly looks good to me) ...
>
> Paul
>
> xiao li - Sun Microsystems - Beijing China wrote:
>>
>> updated webrev:
>> http://cr.opensolaris.org/~xl222276/sg3utils/
>
> === Start of Comments ===
>
> 1. usr/src/cmd/sg3_utils/METADATA
>    Make the NAME: field more descriptive, eg. what is sg3 ?
Done, I've modified the NAME field.
>
> 2. usr/src/cmd/sg3_utils/Makefile.sfw
>    Do you need the lines ..
>     53  find $(VER) -type d -exec /usr/bin/chmod 755 "{}" \;
>     54  find $(VER) -type f -exec /usr/bin/chmod ugo+r "{}" \;
>    if not remove them
Removed.
>
>    Change line ...
>     48  ./configure --prefix=/usr)
>    so it uses the predefined prefix in Makefile.master to
>    something like ..
>     ./configure $(CONFIGURE_OPTIONS))
Done.
>
> 3. usr/src/cmd/sg3_utils/install-sfw
>    Change ..
>     1 #! /usr/bin/ksh
>    to ..
>       #! /usr/bin/ksh93
Fixed.
>
>    Have you run '/usr/bin/ksh93 -n install-sfw' on the script
>    to ensure it doesn't show up any errors or warnings?
Yes, I've checked.
>
>    Remove the double-space in the CDDL header (check other
>    files to), eg. change
>      #  ....
>    to
>      # ....
Done, also checked other files.
>
> 4. usr/src/pkgdefs/SUNWsg3utilsr/copyright
>      & usr/src/pkgdefs/SUNWsg3utilsu/copyright
>    Do you need to put the full licence in here ? (I prefer
>    it as it is though)
I saw that a lot of other packages are doing things this way, should I 
keep it like them?
>
> 5. usr/src/pkgdefs/SUNWsg3utilsr/pkginfo.tmpl
>    You could change DESC= to ..
> DESC="sg3 - package of utilities for sending SCSI commands 1.25 (root)"
Done.
>
> 6. usr/src/pkgdefs/SUNWsg3utilsu/depend
>    Have you checked you have no other dependencies using
>    the dependency checker script?
Yes, I've checked using "make check_deps" and modified the depend file.
>
> 7. usr/src/pkgdefs/SUNWsg3utilsu/pkginfo.tmpl
>    You could change DESC= line as in (5)
Done.
Thank you for your comments and advices.
-Xiao
>
> === End of Comments =====
>