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 2. usr/src/cmd/php5/Solaris/php.1.sunman Do the copyright lines comply to the normal sun copyright format? 3. usr/src/cmd/php5/Solaris/php5.2.conf Copyright year is wrong 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' 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. 6. usr/src/cmd/php5/dtrace-1.0.3.tar.gz & usr/src/cmd/php5/idn-0.1.tar.gz & usr/src/cmd/php5/memcache-2.2.3.tar.gz Have these really changed? 7. usr/src/cmd/php5/patches/dtrace.patch & usr/src/cmd/php5/patches/idn_config.patch Have these really changed? 8. php-5.2.6 bz2 I don't see this file in your webrev, or the removal of the old one. 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 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 11. usr/src/pkgdefs/SUNWphp52d/prototype_com & other SUNWphp*/prototype_com files Do all these files really need the write permissions bit set? 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. 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? 15. usr/src/pkgdefs/SUNWphp52r-pear/prototype_com Should all these files really be in a 'root' pkg? eg. *.html, etc. 16. usr/src/pkgdefs/SUNWphp52r-pear/depend Should a 'root' pkg really depend on SUNWphp52r & SUNWphp52u phew there's a lot of it!!! === End of Comments ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
