[included both webstack as well as sfwnv .. ] thanks a million for taking time to review. pl. see my response inline...
pl. find my updated webrev - incorporating some of your review comments within http://cr.opensolaris.org/~sn123202/php526.2/webrev/ I would also appreciate from the community if I can get one more review with my updated webrev... I would like to make it build 99 - so your earliest response will be much appreciated thanks sriram Paul Cunningham wrote: > Sriram, > > Some comments below ... > > Paul > > Sriram Natarajan wrote: > >> HI >> Apologize on the cross posting. Couple of days back, I requested for >> code review on webstack-discuss at opensolaris.org alias regarding php >> version upgrade to 5.2.6 . For some reason, the posting never showed up >> in our Jive based forum site. Hence, I am not able to refer to the >> original posting. As you all know, I need at least 2 reviews to >> proceed further. I would really appreciate, if you can take a moment >> and reply with your valuable feedback. >> >> For your reference, >> - PHP 5.2.6 ARC case can be referred from >> http://pl.opensolaris.org/os/community/arc/caselog/2008/538/materials/php526-txt/ >> >> >> >> - Accompanying web rev for PHP 5.2.6 version update to accommodate the >> file layout and package rename change can be referred from >> http://cr.opensolaris.org/~sn123202/php526.1/webrev >> > > === Start of comments === > > 1. usr/src/Makefile > You don't seem to have changed anything in this file > > corrected. > 2. usr/src/cmd/php5/Solaris/php.1.sunman > Do the copyright lines comply to the normal > sun copyright format? > > this is what we did for apache 2.2 as well as php 5.2.4 > 3. usr/src/cmd/php5/Solaris/php5.2.conf > Copyright year is wrong > done > 4. usr/src/cmd/php5/Makefile.sfw > Use "env - ..." rather than "env ..." > > Put the '-rm -rf' list all on one line, eg. > > '-rm -rf XX YY BB CC' > done > 5. usr/src/cmd/php5/install-php5 > As you are changing this file you might want to > look at 'Roland Mainz' comments on other people's > install-* scripts and apply those comments - look > back through sfwnv-discuss for them. > - ran with ksh93 -n - updated to use ksh93 - updated to have errexit within the script > 9. usr/src/pkgdefs/SUNWapch22m-php52/depend > & all other usr/src/pkgdefs/SUNW*/depend files > Move the copyright lines to after the 'CDDL HEADER END' > header > done > 10. usr/src/pkgdefs/SUNWphp52d/pkginfo.tmpl > & usr/src/pkgdefs/SUNWphp52r-mysql/pkginfo.tmpl > & usr/src/pkgdefs/SUNWphp52r-pgsql/pkginfo.tmpl > & maybe other pkginfo.tmpl files > You could remove the '5.2.x' from the NAME= line > done. > 11. usr/src/pkgdefs/SUNWphp52d/prototype_com > & other SUNWphp*/prototype_com files > Do all these files really need the write permissions > bit set? > changed to 444 > 12. usr/src/pkgdefs/SUNWphp52r/depend > Isn't this just the default set of depends, so just > add DATAFILES= depend to the pkgs Makefile and > remove this. > done > 13. usr/src/pkgdefs/SUNWphp52u-mysql/prototype_i386 > Copy right year is wrong > > 14. usr/src/pkgdefs/SUNWphp52u/depend > Should this have a dependency on SUNWphp52r? > done > 15. usr/src/pkgdefs/SUNWphp52r-pear/prototype_com > Should all these files really be in a 'root' pkg? > eg. *.html, etc. > I am afraid so. We cannot split this as pear component requires these along with the rest to reside under /var. this would allow third party components to be downloaded under /var/php/5.2/pear. > 16. usr/src/pkgdefs/SUNWphp52r-pear/depend > Should a 'root' pkg really depend on SUNWphp52r > & SUNWphp52u > well, pear is not usable on its own. it does need the binary like 'php' to be delivered. pear is delivered as 'root' component only because to allow downloading third party components in /var. we don't want our customers downloading third party components in /usr thanks a million !!! - sriram > === End of Comments ===== > >
