Thanks Paul, I've updated the webrev according to all your comment tips, please refresh the page and see how it going?
Thanks. Paul Cunningham wrote: > Robin, > > See comments below ... > > Paul > > Robin Guo wrote: >> >> I just put a code review for tcpdump package porting, >> would you or somebody else have time to take a look on it? >> >> The webrev is at http://cr.opensolaris.org/~robinguo/tcpdump/ > > === Start of Comments ==== > > 1. CDDL HEADER and top of files > Please updated as per those in ... > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > > 2. usr/src/cmd/tcpdump/METADATA > Add missing fields as in ... > http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines > > 3. usr/src/cmd/tcpdump/Makefile.sfw > Extract the VER= info from the METADATA, see recent > integrations for examples. > > Roland Mainz wrote: > > use "env - ..." and not "env ..." in the Makefiles to > > make sure "configure"&&"make" only see the env variables > > they should really get (and not pick-up any random > > env variable) > > use either $(SHELL) or /usr/bin/bash for "configure" > > calls (so we know which one is used and "configure" > > doesn't pick one itself) > > Change the following so that it uses the predefined > --prefix= value from Makefile.master, and also use the > CONFIGURE_OPTIONS method of setting and using configure > options ... > 54 ./configure --prefix=/usr \ > 55 --enable-ipv6=yes ) > So something like ... > CONFIGURE_OPTIONS += --enable-ipv6=yes > etc > .... > .... > ./configure $(CONFIGURE_OPTIONS) > > Do you really need the line ? ... > 41 @find . -name core -exec rm -f {} \; > if not remove it > > Add line after '29 VER= ...' ... > TARBALL=$(VER).tar.gz > then replace throughout ... > $(VER).tar.gz > with ... > $(TARBALL) > > 3. Portions Copyright in various files > Remove the 'Portions Copyright' lines, eg ... > # /* Portions Copyright 2007 ShivaKumar GN */ > > 4. usr/src/cmd/tcpdump/install-sfw > Change as .. > Roland Mainz wrote: > > use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* > > and add a $ set -o errexit # at the beginning and > > replace ". ${SRC}/tools/install.subr" with > > "source ${SRC}/tools/install.subr" (the idea is to > > catch failures in the script and abort it at that point, > > right now the script will just continue) > > Pass the PKGVERS= info in from your Makefile.sfw (extracted > from METADATA) as an environment var or as an option. > > 5. usr/src/pkgdefs/SUNWtcpdump/copyright > Add the package owner copyright lines (from src files) > at the top, see example .. > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > > > 6. usr/src/pkgdefs/SUNWtcpdump/depend > Move Copyright lines to after the CDDL HEADER END > header + as in (1) > > 7. usr/src/pkgdefs/SUNWtcpdump/pkginfo.tmpl > Add version number to end the DESC= line, eg. ... > DESC="....... (4.0.0)" > > === End of Comments ====== -- Regards, Robin Guo, Xue-Bin Guo Solaris Kernel and Data Service QE, Sun China Engineering and Reserch Institute Phone: +86 10 82618200 +82296 Email: robin.guo at sun.com Blog: http://blogs.sun.com/robinguo
