Ok. Lots has changed since this first round of code review. First the PSARC has a +1 so the architecture changes should be set.
I'll respond to all 3 of April's emails plus there's a new package, php-common, which holds common components. Namely a generic man page and a configuration file. The full new webrev is here: http://jurassic.us.oracle.com/net/mogo/builds3/cmohrman/userland_php-5_3_10/webrev/ The delta webrev including April's comments and new stuff is here: http://jurassic.us.oracle.com/net/mogo/builds3/cmohrman/userland_php-5_3_10/webrev-delta-r2/ Plus compare these files manually for something more meaningful besides NEW: cd /net/mogo.us.oracle.com/builds3/cmohrman/userland_php-5_3_10/components {DIFF} php-5_2/apache-php52.p5m php-5_3/apache-php53.p5m {DIFF} php-5_2/apache-php52.license php-5_3/apache-php53.license Thanks, craig ----- april.c...@oracle.com wrote: > Hi Craig, > > I haven't gotten through all the files. Here's what I have so > far... > > April > > General comment: > - For the license files, please add a component name & version #. > Done. Some components, pear and mysql, are bundled with the php download so I named them php-pear and php-mysql. The other addons are separate so they retain their names. > components/php-5_2/Makefile: > > - Remove this comment: > > 26 ## > 27 ## DO: > 28 ## patch > 29 ## 12_php_7050600.patch > 30 ## can go away once i get hold of 7052015 in b171 > 31 ## > Done > components/php-5_2/APC/Makefile: > > - You might change "5.2" below to "$(PHP_REL)", > as in done with the php 5.3 version: > > 43 $(MKDIR) $(PROTO_DIR)/usr/php/5.2/samples/apc; \ > 44 $(INSTALL) -m 0555 $(SOURCE_DIR)/apc.php \ > 45 $(PROTO_DIR)/usr/php/5.2/samples/apc/apc.php; > \ > I'm going to skip this. 5.2 is working. 5.3 I tried to make things more adaptable to new versions going forward. 5.3 infra-structure will be copied to create 5.4 someday. > components/php-5_3/APC/Makefile: > > - Since you're using gsed, you can use the -i option for in-place > editing > of the php-apc-stats.conf file in the proto dir, instead of editing > a > tmp file first. > > - Also, you'll want to install php-apc-stats.conf with mode 0644, as > done with > php 5.2, since the installed file will be 0644, and I understand > you'd like > to recreate the files in the proto area, although Userland doesn't > > require this. > > 40 $(MKDIR) $(PROTO_DIR)/$(APACHE2_SAMPLES_CONFDIR); \ > 41 $(MKDIR) $(COMPONENT_DIR)/tmp; \ > 42 $(GSED) -e "s@<<VERSION>>@$(PHP_REL)@g" \ > 43 < $(COMPONENT_DIR)/../Solaris/php-apc-stats.conf \ > 44 > $(COMPONENT_DIR)/tmp/php-apc-stats.conf; \ > 45 $(INSTALL) -m 0444 > $(COMPONENT_DIR)/tmp/php-apc-stats.conf \ > 46 > $(PROTO_DIR)/$(APACHE2_SAMPLES_CONFDIR)/php$(PHP_REL)-apc-stats.conf; > \ > > Done > components/php-5_3/APC/patches/01-apc_pool.c.patch: > > The original patch for php 5.2 replaced a member of the struct > typedef struct _pool_block > > unsigned char data[0]; > with > unsigned char data[1]; > > probably to deal with a compilation error for the 0 subscript in the > array. > > but the new patch for php 5.3 just adds > > unsigned char data[1]; > > I don't believe this patch is still needed. > Previously in apc_pool.c, there was code > which used "data"; now, the same code seems to calculate a word > boundary offset from the address of the struct instead. > Agree. Thanks. > components/php-5_3/APC/patches/04-apc_sem.c.patch: > > This new line added by the patch: > 16 + apc_eprint("apc_sem_create: semctl(%d,...) > failed:", semid); > > should probably look like > the other error messages that were changed from > apc_eprint("apc_sem_create: semctl(%d,...) failed:", semid); > to > apc_error("apc_sem_create: semctl(%d,...) failed:" TSRMLS_CC, > semid); > between APC versions 3.0.19 and 3.1.9. > Opps. > components/php-5_3/Makefile: > > - Remove this comment: > 26 ## > 27 ## DO: > 28 ## patch > 29 ## 12_php_7050600.patch > 30 ## can go away once i get hold of 7052015 in b171 > 31 ## > > - Change to "5.3": > 38 # PHP 5.2 Done. > > components/php-5_3/extensions-zts.mk: > > - Can use $(GSED) -i option to edit in place instead > 45 $(MV) configure configure_orig; \ > 46 $(GSED) -e > "s@^PHP_EXECUTABLE=.*@PHP_EXECUTABLE=$(COMPONENT_DIR)/../php-sapi/build/i86/sapi/cli/php@" > > \ > 47 < configure_orig > configure ) > Done. > components/php-5_3/extensions.mk: > > - Can use $(GSED) -i option here too > Done. > components/php-5_3/memcache/Makefile: > > - Can use $(GSED) -i option here too > 42 $(MKDIR) $(COMPONENT_DIR)/tmp; \ > 43 $(GSED) -e "s@<<VERSION>>@$(PHP_REL)@g" \ > 44 < $(COMPONENT_DIR)/../Solaris/php-memcache-stats.conf \ > 45 > $(COMPONENT_DIR)/tmp/php-memcache-stats.conf; \ > 46 $(INSTALL) -m 0644 > $(COMPONENT_DIR)/tmp/php-memcache-stats.conf \ > 47 > $(PROTO_DIR)/$(APACHE2_SAMPLES_CONFDIR)/php$(PHP_REL)-memcache-stats.conf; > \ > Done. > components/php-5_3/php-apc.p5m: > > - Should these editable php 5.3 files > be preserve=renamenew, like their php 5.2 counterparts, > instead of preserve=renameold? > > etc/apache2/2.2/samples-conf.d/php5.3-apc-stats.conf > etc/php/5.3/conf.d/apc.ini > etc/php/5.3/zts-conf.d/apc.ini > I guess you are right. Remember that php5.3 is a NEW component so I was worrying about the case where the user installed a php5.3 on their own, from sunfreeware.com for instance, but that seems unlikely and they should also know enough to uninstall that one first before installing ours. So I'll change to renamenew. > components/php-5_3/php-pear.p5m: > > <transform file path=var/php/.* -> default preserve renameold> > preserves a lot of files under /var/php. > Should this be renameold? The php 5.2 version uses renamenew > The theory is anything under /var is potentially editable so yeah it's a lot. Changed to renamenew. > > > On 04/20/12 09:53 AM, Craig Mohrman wrote: > > php 5.3.10 is being added alongside php 5.2.17. > > > > I'm not quite through PSARC. Some minor nits surrounding > > the version control via an IPS mediator. PSARC/2012/067 > > But as I'm trying to get this into B15, pretty please Mike hold the > gate, > > I want to get this review started now because it's big. > > > > Here is the normal webrev: > > > http://jurassic.us.oracle.com/net/mogo/builds3/cmohrman/userland_php-5_3_10/webrev/index.html > > > > It's not very useful because most of the files are "new" while > > in fact they are just tweaked copies of php5.2 files. > > So I wrote a little script to compare these files directly > > which should be helpful. > > > > /home/cmohrman/php-cr > > or > > /net/hs-usca-01.sfbay/export/home1/12/cmohrman/php-cr > > > > This uses a graphical diff tool, filemerge or meld, and a list of > files > > where you choose where to start. > > You can interrupt the script at any point and start over. > > > > If you want to use meld you'll need pkg:/developer/meld installed. > > It's slightly nicer because it shows graphically where lines moved > to > > if not far away. > > > > The script won't review all files yet because of file renames so > > I'm working on a script for that now. > > > > But this will keep someone busy for a while as is. > > > > Many thanks to you brave soles who help review this. > > craig > > _______________________________________________ > > userland-discuss mailing list > > userland-discuss@opensolaris.org > > http://mail.opensolaris.org/mailman/listinfo/userland-discuss _______________________________________________ userland-discuss mailing list userland-discuss@opensolaris.org http://mail.opensolaris.org/mailman/listinfo/userland-discuss
