As your RTI advocate, I normally do a code review anyway, but you should
not rely on that for your code review. If you use me as a code
reviewer, you will need to use another advocate for your RTI. At any
rate, here are my comments from a quick pass through your webrev:
usr/src/lib/Makefile:
* are there dependency relationships between libmemcached,
memcached, and memcache-java that need to be expressed here?
usr/src/lib/libmemcached/Makefile.sfw:
* General comment, there are several variables in Makefile.master
that you should be using
* lines 42-51, 56-66 do you need to set all of these variables in
the calling environment? just asking
* line 50 Do you need $(LDFLAGS) here ? They are defined in
CXXLDFLAGS right above and presumably used by CXXLD
* lines 64, 65 Do you need $(LDFLAGS64) and $($(MACH64)_XARCH) here?
ditto
* lines 51, 66 use MAKE=$(CCSMAKE)
* line 87, 97 see pod2man comment below
* line 115 can be removed
usr/src/lib/libmemcached/install-sfw:
* In general, I would prefer to see you use the components "install"
target instead of using install-sfw to cherry pick bits from the
build tree.
* lines 46, 48-56 copy files out of the libtool .libs directory. I
would verify that the files you are cherry picking don't have
references to your workspace in their RUNPATH.
usr/src/lib/libmemcached/install-sfw-64:
* In general, I would prefer to see you use the components "install"
target instead of using install-sfw to cherry pick bits from the
build tree.
* lines 39-41 copy files out of the libtool .libs directory. I
would verify that the files you are cherry picking don't have
references to your workspace in their RUNPATH.
usr/src/lib/libmemcached/pod2man:
* Is there something wrong with the pod2man delivered in Solaris?
Please remove this one and use the delivered version instead.
usr/src/lib/memcached-java/install-sfw:
* line 42 This file should not be writable
usr/src/lib/memcached/Makefile.sfw
* General comment, there are several variables in Makefile.master
that you should be using
* You might split the build of your isaexec bits into it's own
target(s) with comments. It was a bit confusing at first,
particularly since it's called memcached.
usr/src/lib/memcached/Solaris/memcached:
* no change, reset it and don't put it back.
usr/src/lib/memcached/dtrace.patch:
* Are you working on getting these changes upstream?
usr/src/lib/memcached/sunman-stability:
* no changes, it shouldn't be in the webrev and shouldn't go back.
usr/src/pkgdefs/SUNWmemcached-java/depend
* seems odd that you don't need java
usr/src/pkgdefs/SUNWmemcached/Makefile
* no change, reset it and don't put it back.
usr/src/pkgdefs/SUNWmemcached/copyright
usr/src/pkgdefs/SUNWmemcachedr/copyright
* did someone really give up their copyright/license? Is there a
new OSR?
usr/src/pkgdefs/SUNWmemcached/depend
usr/src/pkgdefs/SUNWmemcachedr/depend
* no change, reset it and don't put it back.
usr/src/pkgdefs/SUNWmemcachedr/prototype_i386
usr/src/pkgdefs/SUNWmemcachedr/prototype_sparc*
*
* no change, reset it and don't put it back.
General comment, please collapse your sccs deltas (wx reci, ...)
-Norm
Victor Kirkebo wrote:
> Hi,
> We would really like someone to review this integration. It would be
> greatly appreciated if it could be done by the end of the week.
> The webrev is located at
> http://cr.opensolaris.org/~vk136562/memcached-1.2.5/.
>
> Important new features in this memcached release are:
> - 64 bit version of memcached
> - Inclusion of Dtrace probes
> - Inclusion of API for Java platform.
> - Replacement of libmemcache C API with libmemcached C API
>
> Thanks
>
