Please see my comments inline.
Norm Jacobs wrote:
>
> 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?
No. The server (memcached) and the clients (libmemcached and
memcache-java) can be installed on separate systems.
>
> usr/src/lib/libmemcached/Makefile.sfw:
>
> * General comment, there are several variables in Makefile.master
> that you should be using
Added SFW_PATH, CCSMAKE and removed .SUFFIXES, CCREGSYM
> * lines 42-51, 56-66 do you need to set all of these variables in
> the calling environment? just asking
I've cleaned this up.
> * line 50 Do you need $(LDFLAGS) here ? They are defined in
> CXXLDFLAGS right above and presumably used by CXXLD
I've cleaned this up.
> * lines 64, 65 Do you need $(LDFLAGS64) and $($(MACH64)_XARCH) here?
> ditto
I've cleaned this up.
> * lines 51, 66 use MAKE=$(CCSMAKE)
Done
> * line 87, 97 see pod2man comment below
Done
> * line 115 can be removed
Done
>
> 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.
Done
> * 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.
Done... and no more cherry picking from .libs.
>
> 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.
Done
> * 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.
Done... and no more cherry picking from .libs.
>
> 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.
Removed
>
> usr/src/lib/memcached-java/install-sfw:
>
> * line 42 This file should not be writable
Done
>
> usr/src/lib/memcached/Makefile.sfw
>
> * General comment, there are several variables in Makefile.master
> that you should be using
Added SFW_PATH, CCSMAKE and removed .SUFFIXES, CCREGSYM
> * 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.
Done
>
> usr/src/lib/memcached/Solaris/memcached:
>
> * no change, reset it and don't put it back.
I fixed a formatting error in the #ident string
>
> usr/src/lib/memcached/dtrace.patch:
>
> * Are you working on getting these changes upstream?
Yes. It will happen.
>
> usr/src/lib/memcached/sunman-stability:
>
> * no changes, it shouldn't be in the webrev and shouldn't go back.
Removed.
>
> usr/src/pkgdefs/SUNWmemcached-java/depend
>
> * seems odd that you don't need java
Added SUNWj5rt.
>
> usr/src/pkgdefs/SUNWmemcached/Makefile
>
> * no change, reset it and don't put it back.
I fixed a formatting error in the #ident string
>
> usr/src/pkgdefs/SUNWmemcached/copyright
> usr/src/pkgdefs/SUNWmemcachedr/copyright
>
> * did someone really give up their copyright/license? Is there a
> new OSR?
We removed the client libmemcache from SUNWmemcached. The new client
libmemcached is now in SUNWlibmemcached.
>
> usr/src/pkgdefs/SUNWmemcached/depend
> usr/src/pkgdefs/SUNWmemcachedr/depend
>
> * no change, reset it and don't put it back.
I fixed a formatting error in the #ident string
>
> usr/src/pkgdefs/SUNWmemcachedr/prototype_i386
> usr/src/pkgdefs/SUNWmemcachedr/prototype_sparc*
> *
>
> * no change, reset it and don't put it back.
I fixed a formatting error in the #ident string
>
>
> General comment, please collapse your sccs deltas (wx reci, ...)
Done
>
> -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
>>
>
Thanks,
Victor
