Here are some of my comments :
---------------------------------------------------------
1. Also to me, in following line, $(MEMCACHE_DIR).tgz is not very unreadable.
+$(MEMCACHE_DIR)/configure: $(MEMCACHE_DIR).tgz
I would rather define a variable e.g
MEMCACHE_TAR_FILE=memcache-$(MEMCACHE_VERSION).tgz
and then write make rule like :
$(MEMCACHE_DIR)/configure: $(MEMCACHE_TAR_FILE)
gzip -dc $(MEMCACHE_TAR_FILE) | ...
--------------------------------------------------------------
2. Following two lines can be more concisely be written :
Current
+ gzip -dc $(MEMCACHE_DIR).tgz | tar xopf -
+ touch $(MEMCACHE_DIR)/configure
Suggested :
+ gzip -dc $< | tar xopf -
+ touch $@
--------------------------------------------------------------
3. I would rather prefer to save configure options in a separate varaible e.g.
MEMCACHE_CONFIG_OPTION=--disable-debug \
--enable-memcache \
--with-php-config=../php-config-proto \
--with-zlib-dir=$(ROOT)/usr )
$(MEMCACHE_DIR)/config.status: $(MEMCACHE_DIR)/configure
(cd $(MEMCACHE_DIR); \
env $(ENVLINE) ../phpize-proto; \
env $(ENVLINE) sh ./configure $(MEMCACHE_CONFIG_OPTION)
--------------------------------------------------------------
4. There is small cosmetic change comments :
+ env $(ENVLINE) ../phpize-proto; \
+ env $(ENVLINE) sh ./configure \
+ --disable-debug \
+ --enable-memcache \
+ --with-php-config=../php-config-proto \
+ --with-zlib-dir=$(ROOT)/usr )
In above mentioned in first two lines, there is 1 tab, followed by 4 spaces,
while next four lines contain 2 tabs. I believe, we can make it more consistent.
As far as Makefiles are concerned, we can safely use spaces after 1st tab.
The issue is there in other places too in php5/Makefile.sfw.
---------------------------------------------------------
BTW memcache-2.2.3 version is also out.
http://pecl.php.net/package/memcache
Regards,
Basant.
On Fri, Feb 01, 2008 at 07:20:58PM -0800, Sriram Natarajan wrote:
> Hi
> Can you kindly take a moment and review on the upcoming PHP memcached
> module integration related changes. Please find below the 'webrev'
> output and provide us with your valuable comments. Please note that
> 'memcached' server has been integrated with Nevada build 80.
>
> http://cr.opensolaris.org/~sn123202/6630059/webrev
>
> thanks in advance
> sriram
> _______________________________________________
>
>
> webstack-discuss mailing list
> webstack-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/webstack-discuss
