Lin, On Wed, 2008-07-09 at 15:23, Lin Guo wrote: > > Thanks a lot for your comments. Please see my answers below. > > I have corrected all the errors. The latest webrev is at : > http://cr.opensolaris.org/~linguo/snack/ > > Could you please help review the latest webrev.
Looks okay to me, but see comments inline below .. > Thanks a lot for your time. That's okay Paul :-) > > > Paul Cunningham wrote: > > 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. > > > Done. > > 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. > Changed to '--prefix= /usr' > I am not sure if I understood CONFIGURE_OPTIONS correctly. It seems > that this option is not suitable for my package as 'cups', since I > have to run configure script twice to make both 32bit and 64bit > libraries. I think there are various examples now where people have used it for both 32 & 64 bit (I can't remember where though). > If using CONFIGURE_OPTIONS, then the Makefile.sfw will be like: > ... > CONFIGURE_OPTIONS += --with-tcl=$(ROOT)/usr/lib / > CONFIGURE_OPTIONS += --with-tk=$(ROOT)/usr/lib / > ./configure $(CONFIGURE_OPTIONS) > ... > CONFIGURE_OPTIONS += --with-tcl=$(ROOT)/usr/lib/64 / > CONFIGURE_OPTIONS += --with-tk=$(ROOT)/usr/lib/64 / > ./configure $(CONFIGURE_OPTIONS) > ... > Is it right? I have to define the parameters twice. What's your idea? I think that is about right, but for 32 & 64 bit I think its been done something like ... CONFIGURE_OPTIONS32 = $(CONFIGURE_OPTIONS) CONFIGURE_OPTIONS32 += --with-tcl=$(ROOT)/usr/lib CONFIGURE_OPTIONS32 += --with-tk=$(ROOT)/usr/lib CONFIGURE_OPTIONS64 = $(CONFIGURE_OPTIONS) CONFIGURE_OPTIONS64 += --with-tcl=$(ROOT)/usr/lib/64 CONFIGURE_OPTIONS64 += --with-tk=$(ROOT)/usr/lib/64 ...... ./configure $(CONFIGURE_OPTIONS32) ...... ./configure $(CONFIGURE_OPTIONS64) ...... but check to see how other people have done it. > > > Why use 'all: real-all' when it could just be > > 'all: all32 all64' ? > > > Removed 'real-all' > > I think '-xstrconst' is defined in Makefile.master > > now so use that instead. > > I didn't find it in Makefile.master. you are right, my mistake I thought it had been put in there - maybe some one should put it in. So with out that below looks okay. > I am also looking for something like '$(XSTRCONST)'. > The result is like: > ./links/Makefile.sfw:XSTRCONST = -xstrconst > ./links/Makefile.sfw:CFLAGS += $(XSTRCONST) > > > 3. usr/src/lib/snack/install-sfw > > Should 'VER=snack2.2.10' be 'VER=snack-2.2.10' ? > > > Changed it to 'VER=snack-2.2.10'. > > Aren't 'SHAREDOCDIR=' and 'DOCDIR=' the same thing, if so > > just have one of them. > > > Removed "SHAREDOCDIR". > > 3. usr/src/lib/snack/install-sfw-64 > > Should 'VER=snack2.2.10-64' be 'VER=snack-2.2.10-64' ? > > > > > Changed to 'VER=snack-2.2.10-64" > > 4. usr/src/lib/snack/pkgIndex.tcl > > Should this have the CDDL header and 'Copyright' lines ? > > > In general, the file "pkgIndex.tcl" is created by pkg_mkIndex utility > for Tcl Extensions, that's why such files have no copyright. To > support both 32bit and 64bit libraries, I modified the file a > little,but most of the content are created by the utility. So I didn't > add a copyright. > > 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? > > > Fixed. > > 6. usr/src/lib/snack/snack.1t.sunman > > Did you write this - does it need the CDDL header and > > 'Copyright' lines ? > > > This man page was written by myself, so I added CDDL header and > 'Copyright' lines. Did you add this - it doesn't seem to be in the webrev -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Computer Products
