Steven,
This looks okay to me now on a quick skip through; you need to resync
usr/src/cmd/Makefile with the gate though (so it doesn't look as though
you are try to change other stuff).
Paul
Steven Le wrote:
> On 09/23/08 03:03, Paul Cunningham wrote:
>> Steven,
>>
>> I took another quick look - you might also want to change the
>> following ...
>>
>> 1. usr/src/cmd/bonnieplus/METADATA
>> Add the BUGTRAQ and SUPPORT info, eg. ...
>> SUPPORT: ????
>> BUGTRAQ: solaris/utility/????
>>
>> 2. usr/src/cmd/bonnieplus/install-sfw
>> (I forget if you responded to this ..)
>> You might want to apply these comments ..
>>
>> Roland Mainz wrote:
>> > - Please 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)
>>
>> 3. usr/src/cmd/bonnieplus/Makefile.sfw
>> (again I forget if you responded to this ..)
>> You might want to apply these comments ...
>>
>> Roland Mainz wrote:
>> > - use "env - ..." and not "env ..." in the Makefiles
>> > to make sure "configure" & "make" only see the env
>> > variables they should really get (and not pick-up
>> > any random env variable)
>> > - use either $(SHELL) or /usr/bin/bash for "configure"
>> > calls (so we know which one is used and "configure"
>> > doesn't pick one itself)
>>
>> Christopher Mi wrote:
>> > Use the method define in Makefile.master
>> > since you have a standard METADATA file.
>> >
>> > VER =$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)
>> > TARBALL =$(VER).tar.bz2
>>
>> Does it really core dump during the 'all:' rule, if not
>> remove the 'find ... core ...'. If it does, why does it?
>>
>> 4. depend
>> Did you check you have no other dependencies?
>>
>>
>> Everything else looks okay to me
>>
>> Paul
>>
>>
>>
>> Steven Le wrote:
>>> Hi all,
>>> I have fixed all the problems that you suggested me to do. The
>>> up2date files are at:
>>> http://cr.opensolaris.org/~steven84/bonnieplus/
>>>
>>> Please email if there is anything
>>> Note: We need your final approval in order to put back, please help
>>> me as soon as you can.
>>>
>>> 1st review: FIXED
>>>
>>> * Create webrev with the CR#6741736 FIXED
>>> * usr/benchmarks needs to added as part of usr/src/Targetdirs FIXED
>>> * in METADATA mention URL of bonnie++ FIXED
>>> * bonnie++ should be installed in usr/sbin instead of usr/bin FIXED.
>>> NOTE: our sponsors request us to put into usr/benchmarks
>>> * zcav is missing in prototype_com and also add manpage FIXED
>>> * in usr/src/pkgdefs/SUNWbonnieplus/prototype_com add symlinks for
>>> bon_csv2txt bon_csv2html FIXED
>>> * in usr/src/pkgdefs/SUNWbonnieplus/prototype_sparc add '#' between
>>> copyright and make sure ident string is correct FIXED
>>> * remove usr/src/pkgdefs/SUNWbonnieplus/pkginfo file FIXED
>>>
>>>
>>> 2nd review: FIXED
>>> === Start of Comments ===
>>>
>>> 1. usr/src/cmd/bonnieplus/METADATA
>>> Add line for where the source code came from. 'URL: ...'
>>>
>>> 2. usr/src/cmd/bonnieplus/install-sfw
>>> You may want to apply 'Roland Mainz' comments, on other
>>> people's install-* scripts to this. You can find those
>>> comments in the sfwnv-discuss archives.
>>>
>>> BINDIR= is not used so you could delete
>>>
>>> 3. usr/src/cmd/bonnieplus/Makefile.sfw
>>> You may want to use "env - ..." rather than "env ..."
>>> so its doesn't pick-up any random env variable.
>>>
>>> 4. usr/src/pkgdefs/SUNWbonnieplus/Makefile
>>> Add line space after '#ident ...' line
>>>
>>> 5. usr/src/pkgdefs/SUNWbonnieplus/depend
>>> This looks like the default 'depend' file; if you have
>>> no other dependencies you should remove this file and
>>> add 'DATAFILES= depend' to your SUNWbonnieplus/Makefile.
>>> But have you checked you have no other dependencies with
>>> the dependency checker script?
>>>
>>> 6. usr/src/pkgdefs/SUNWbonnieplus/pkginfo
>>> Remove this files as it is generated from the pkginfo.tmpl
>>> file.
>>>
>>> 7. usr/src/pkgdefs/SUNWbonnieplus/prototype_sparc
>>> Add line space before '#ident ...' line
>>>
>>> 8. usr/src/pkgdefs/SUNWbonnieplus/prototype_i38
>>> & usr/src/pkgdefs/SUNWbonnieplus/prototype_sparc
>>> '#ident ...' line says ...
>>> # ident "@(#)pkginfo.tmpl ...
>>>
>>> === End of Comments =====
>>>
>>
> Hi,
> Thanks for your review. I have changed files as you have recommended.
> Could you help me to review again to see as if I need anything? We are
> trying to put back bonnie++ next Tuesday.
> http://cr.opensolaris.org/~steven84/bonnieplus/
>
> Note: C-team needs your final approval in order to complete the review.
>
--
----------------------------------------------------------------------
Paul Cunningham
Software Engineer
Tadpole Computer Products
