Hi Lin, See my comments below from my quick skip through ...
Paul On Sun, 2008-07-06 at 14:09, Lin Guo wrote: > > Please help code review the inclusion of snack 2.2.10 into the sfw > consolidation. > > The webrev is at: > http://cr.opensolaris.org/~linguo/snack/ > === Start of Comments ==== 1. usr/src/Targetdirs & usr/src/lib/Makefile Keep in alphabetical order where possible. 2. usr/src/lib/snack/Makefile.sfw Use the predefined 'prefix=' from Makefile.master, via CONFIGURE_OPTIONS, eg. 'configure $(CONFIGURE_OPTIONS)', see 'cups' for an example. I don't think it should be '--prefix=${ROOT}/..' anyway as ROOT is the proto area. Why use 'all: real-all' when it could just be 'all: all32 all64' ? I think '-xstrconst' is defined in Makefile.master now so use that instead. 3. usr/src/lib/snack/install-sfw Should 'VER=snack2.2.10' be 'VER=snack-2.2.10' ? Aren't 'SHAREDOCDIR=' and 'DOCDIR=' the same thing, if so just have one of them. 3. usr/src/lib/snack/install-sfw-64 Should 'VER=snack2.2.10-64' be 'VER=snack-2.2.10-64' ? 4. usr/src/lib/snack/pkgIndex.tcl Should this have the CDDL header and 'Copyright' lines ? 5. usr/src/pkgdefs/SUNWsnack/prototype_i386 & usr/src/pkgdefs/SUNWsnack/prototype_sparc Shouldn't the 's none ..' line come after the 'd none ..' line? 6. usr/src/lib/snack/snack.1t.sunman Did you write this - does it need the CDDL header and 'Copyright' lines ? === End of Comments ======
