Please see my comments inline.
Danek Duvall wrote:
> On Tue, Mar 11, 2008 at 01:15:03PM +0100, Victor Kirkebo wrote:
>
>
>> http://cr.opensolaris.org/~vk136562/memcached-1.2.5/
>>
>
> Mike recently putback fixes for 6657798. You need to follow the direction
> he set there. In particular, you've unapplied some of his changes in
> existing makefiles.
>
Added SFW_PATH, CCSMAKE and removed .SUFFIXES, CCREGSYM
> When is pod2man used? And why are you supplying your own instead of using
> the one on the system?
>
pod2man removed. Now using /usr/perl/5.8.4/bin/pod2man.
> For files that are passed through the C compiler, "#pragma ident" and
> "#ident" tell the compiler to put bits in the binary's comment section.
> The ident strings in all other files are nothing more than comments, and
> should look like all the rest of the comments in the file. That is, no
> "pragma" and follow the usual whitespace convention of "# ident". Also
> make sure that the ident lines are correctly unexpanded and are using the
> right %-expandos and whitespace. Correct examples can always be found in
> /ws/onnv-gate/usr/src/prototypes. usr/src/lib/memcached/Solaris/memcached
> looks a little fishy.
>
Done.
> memcached-java has no build process? You're just unpacking the archive to
> find the configure script, but then not running it?
>
Now rebuilding the jar file and javadocs before installing.
> Solaris/memcached is just moving, so you don't need to update its
> copyright. There are a couple of other files you've done nothing except
> update the copyright, too.
>
These had #ident string formatting issues that have been fixed.
> I didn't review memcached before; is there a reason you're carrying around
> a copy of the memcached man page, rather than modifying the existing one
> with a sed script the way most of the rest of the components do? You still
> have the sed script, and it looks like it's still invoked in your install
> script ... so does it add all the extra Sun stuff a second time?
>
We have written our own man page. It's quite different from the
community man page.
> In memcached/install-sfw, you're copying the rbac (not smf, btw) files into
> place, even though you don't completely own them. Mike, is this okay with
> you? It seems like this should be done in a more common location.
>
Removed copying of rbac files. CR 6675390 will take care of this.
> In memcached/install-sfw-64, you set MANSCRIPT, even though you're not
> installing any manpages.
>
Removed MANSCRIPT.
> SUNWlibmemcached/depend: this is identical to the common depend file; you
> should use that one (via DATAFILES in Makefile) instead.
>
Done.
> SUNWmemcached-java/depend: same comment, though shouldn't you have a
> dependency on one of the java packages (SUNWj6rt or something)?
>
SUNWj5rt added.
> SUNWmemcached/depend: shouldn't this now depend on SUNWlibmemcached?
>
No, libmemcached is a client. No dependency either way.
> Danek
>
Thanks,
Victor
