Thanks, Paul. I've addressed your comments. Please see my response in lines. Here is the new webrev for review: http://cr.opensolaris.org/~robinluo/ettcp/ The incremental webrev is: http://cr.opensolaris.org/~robinluo/ettcp-incr/
Paul Cunningham : > Robin, > > See my comments below ... > > Paul > > tian robin luo wrote: >> >> I'm porting package "ettcp" to OpenSolaris, it is a tool for measuring >> max TCP/UDP bandwidth performance. >> Could you please help to review the code? The webrev is at: >> http://cr.opensolaris.org/~robinluo/ettcp/ > > 1. usr/src/pkgdefs/SUNWettcp/prototype_sparc > Line ... > 46 # List files which are I386 specific here > I386 in prototype_sparc file! Done, fixed > > 2. usr/src/pkgdefs/SUNWettcp/depend > Is this the correct set of dependencies, have you checked > it with the package dependency checker tool I updated the set of dependencies with the check-deps.pl tool. > > 3. usr/src/pkgdefs/SUNWettcp/copyright > Do you need to add source owner copyright lines into this > (extracted from the files in the unpacked tarball)? - for example > as in (lines 11-18) ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > Rewrote the copyright in terms of the SUNWmeld/copyright template. > > 4. usr/src/pkgdefs/Makefile > & usr/src/cmd/Makefile > These need resyncing with the gate/clone so it doesn't > look as though you are trying to change other stuff. Have done a bringover on the two files. > > 5. usr/src/cmd/ettcp/Makefile.sfw > Lines ... > 34 CONFIGURE_OPTIONS += PTHREAD_CFLAGS=-pthreads > 35 CONFIGURE_OPTIONS += CC=$(CC) > are these required/used ? Done, removed the two lines. > > Line ... > 44 /usr/bin/gzip -dc $(TARBALL) | tar xopf - > change 'tar' to $(TAR) Modified as your comment. > > Line ... > 38 (cd $(VER); env - CC="$(GCC)" $(CCSMAKE) all) > you are only packaging the 'ettcp' bits of the build, so could > this have just been '$(CCSMAKE) ettcp' (to save time building > the unwanted stuff) ? Modified as your comment. > > Have you changed the src package's 'ettcp-1.0/Makefile' to > get this to build on solaris (it doesn't build for me as-is > from the downloaded tarball using you make invocation) ? Yes, I changed the Makefile, somethings are unused for Solaris. > > 6. usr/src/cmd/ettcp/METADATA > Line ... > 4 LICENSE: OTHER > is 'OTHER' correct? The license of ettcp is "Public Domain", it's not in the list of $(SRC)/tools/metainfo.pl file, so I have to choose OTHER, otherwise it cannot pass the check. > > END Thanks, Robin
