See below ... Paul
Steven Le wrote: > Hi all, > Thanks for your reviews. I have tried to update changes to old > location (http://cr.opensolaris.org/~steven84/bonnieplus) but I got > permission denied. So, I have create a new URL for new changes, could > you check on that? > > New changes: http://cr.opensolaris.org/~steven84/bonnie++/ > > As far as I go, I fixed most the problems. I have some questions from > two reviews (combined): > > > --------START----------- > > * bonnie++ should be installed in usr/sbin instead of usr/bin > * zcav is missing in prototype_com and also add manpage > > We put bonnie++ into usr/benchmarks and create a symbolic link in > usr/bin. Please advise exactly which location bonnie++ will be in. > Zcav is included into bonnie++ as a separate program. If we port > bonnie++, do we have to port zcav also? > 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. >> 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. > > I have read some of examples in SUNW(packages), most of them using > "env \", so I'm confused about using that or "env - \" in makefile.sfw > . Could you give me an example when we use without "-", env will pick > up a random thing? I'm guessing, but most of them should probably have 'env - ...'. I'll let Roland comment more on this. >> usr/src/pkgdefs/SUNWbonnieplus/depend > Where I could get the dependency checker script? Do you have example > of a package that uses DATAFILES=depend. I keep the my depend file > for now. Just grep in your ws for it, maybe something like .. grep "DATAFILES" usr/src/pkgdefs/SUNW*/Makefile | grep depend Don't just include the default 'depend' file explicitly. So only have your own 'depend file if you have extra dependencies. > FIXED BINDIR= is not used so you could delete > 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++ You might also want to add lines for .. SUPPORT: .... BUGTRAQ: .... > 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 indent and format problem in files > > > -----------END--------------------- > > > > On Wed, Sep 10, 2008 at 12:21 AM, Paul Cunningham > <paul.cunningham at tadpole.com> wrote: >> David, >> >> See below for my comments, some of them are probably duplicates of Phadhap's >> comments but I have put them here anyway ... >> >> Paul >> >> David.Fan at sun.com wrote: >>> Hello, >>> >>> We are porting Bonnie++ file system benchmark suite to OpenSolaris. Please >>> take some time to review the webrev at: >>> >>> Webrev: >>> http://cr.opensolaris.org/~steven84/bonnieplus/ >>> >>> RFE: >>> 6741736 P3 bonnie++ 1.03c disk benchmark tool to be included >>> into SFW consolidation >>> >>> Bonnie++ home page: >>> http://www.coker.com.au/bonnie++ >> === 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 ===== > -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
