Paul, Thanks a lot for your review. I've addressed your comments and updated the webrev.
http://cr.opensolaris.org/~grace/dwdiff/ Also, please see my response in lines. Paul Cunningham wrote: > Grace, > > See below for my comments ... > > Paul > > Grace Tang wrote: >> >> I am porting 'dwdiff', a diff program that operates at the word level, >> to OpenSolaris. >> >> Can you please review my code? The webrev is at: >> >> http://cr.opensolaris.org/~grace/dwdiff/ > > === Start of Comments ==== > > 1. usr/src/cmd/dwdiff/METADATA > Make the NAME: field more descriptive. Accepted. > > 2. usr/src/cmd/dwdiff/Makefile.sfw > Extract the version number on 'VER =' from the MEATADATA Accepted. > > Add "env - " before 'configure' and 'make' so it does not > pick up random environment vars. Accepted. > > Explicitly define 'configure --prefix=... " using > CONFIGURE_OPTIONS from Makefile.master, eg. something > like .... > CONFIGURE_OPTIONS += --without-unicode > .... > .... > ./configure $(CONFIGURE_OPTIONS) Accepted. > > Maybe explicitly define the compile to use, CC=... Accepted. > > Maybe use $(CCSMAKE) instead of 'make' Accepted. > > 3. CDDL HEADER and file tops (all files) > Check that they conform to those in ... > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > Updated CDDL HEADER and file tops in all related files. > > 4. usr/src/pkgdefs/SUNWdwdiff/copyright > Do you need the Sun disclaimer? as in ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > Yes, Sun disclaimer is needed. I added it at the beginning. > > 5. usr/src/pkgdefs/SUNWdwdiff/depend > Have you checked the pkg dependencies using the > dependency checker script? Yes, I used depend.sh Chris wrote. > > SUNWlibmsr, is this a dependency of your pkg? Yes, dwdiff links to libm.so which is in SUNWlibmsr. > > 6. usr/src/pkgdefs/SUNWdwdiff/pkginfo.tmpl > Added version at the end of the DESC= line, eg. ... > DESC="................. (1.5.2)" Accepted. > > 7. usr/src/pkgdefs/SUNWdwdiff/prototype_i386 > & usr/src/pkgdefs/SUNWdwdiff/prototype_sparc > To be consistent with other pkgs add comment line .. > # SUNWdwdiff > as in ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/prototype_i386"; > > Accepted. Thanks, - Grace > > === End of comments =====
