usr/src/lib/Makefile - Can you add dbunit to the list alphabetically rather than at the end
usr/src/pkgdefs/SUNWdbunit/depend - So are you happy with the dependencies now? What if someone wants to run dbunit on OpenSolaris with jdk6? What about SUNWant? should that be added? Think of it from the perspective of an end user and consider the most common Use Cases. usr/src/pkgdefs/SUNWdbunit/prototype_com - Packaging the sample as a jar file is a good idea, but I can't see where the jar file was created. Does the dbunit-2.3.0.zip file contain the sample jar file you are packaging? Out of curiousity, where does dbunit-2.3.0.zip come from? I see on sourceforge: the project library jar file, the sources jar file and a dbunit-2.3.0-prj.zip. The latter looks something like your source bundle but it's not the same. Also, why 2.3.0 and not 2.4.3 which is the latest version and which has been around for a month now. Thanks Amanda jin wan - Sun Microsystems - Beijing China wrote: > Hi Paul and Amanda, > > Thanks very much of your detail review again. Please see my answer in > lines. > Updated webrev at: http://cr.opensolaris.org/~bjwancn/dbunit > <http://cr.opensolaris.org/%7Ebjwancn/dbunit> > > Thanks! > -James > > > > On 02/23/09 18:05, Paul Cunningham wrote: >>> >>> On 02/20/09 17:20, Paul Cunningham wrote: >>>> >>>> === Start of Comments ==== >>>> >>>> 1. usr/src/lib/dbunit/METADATA >>>> This doesn't need the CDDL HEADER stuff >>>> >>>> Add missing fields, see ... >>>> http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines >>>> >>> Fixed >> >> The SRC: field is missing > Fixed > >> >>> Add CDDL HEADER stuff >>>> >>> It seems no need to add CDDL HEADER in sunman file according to >>> "http://tas.sfbay/net/sfwnv.sfbay/gates/sfwnv/gate/usr/src/cmd/expect/sunman-stability"; >>> >> >> >> I can't see that link external to the SWAN; but sunman-stability file >> modifies existing man pages, yours is a new man Sun page so I think >> it probably needs it. >> > Right, I use a new man page, and I have added CDDL HEADER in sunman file. > >>>> 7. usr/src/pkgdefs/SUNWdbunit/copyright >>>> Add sun disclaimer and package src copyright stuff at top see .. >>>> "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; >>>> >>> >>> >>> Fixed >> >> you might need to add the pkg owner copyright lines to >> > I'm not sure what's the "pkg owner copyright", should I add "Copyright > (C) 1991, 1999 Free Software Foundation"? which has been listed in the > License text. > >>>> 8. Pkg dependencies >>>> You are using the default 'depend', but have you checked >>>> your pkg has no other dependencies with the dependency >>>> checker script? >> > >> > Add a depend file >> >> You can delete the following in your SUNWdbunit/Makefile now ... >> 31 DATAFILES= depend >> as you have your own depend file. >> > Fixed > >>>> 10. usr/src/pkgdefs/SUNWdbunit/prototype_com >>>> Do you need to deliver the src stuff as part of this pkg, >>>> eg. in ... >>>> usr/share/doc/dbunit/src/* >>>> Just checking? >>>> >>> usr/share/doc/dbunit/src/* is the sample test code for dbunit, not >>> the src of dbunit itself. >> >> Are the ARC happy that you are delivering test src as part of the >> main package rather than in a pkg of its own? >> > I packaged the samples files into a jar file dbunit-2.3.0-sample.jar > and put it in usr/share/doc/dbunit. > > > On 02/23/09 19:50, Amanda Waite wrote: >> Not specifically for this code review, but definitely for the >> putback, you need to re-run 'wx ea' and tidy up the comments. There >> should be one line for every CR (which there's only one of), you have >> duplicate lines along with revision numbers on separate lines. >> > Use wx webrev -O can avoid this issue. > If use /ws/onnv-tools/onbld/bin/webrev -O, it seems it will list all > the revision history in the child workspace. > >> usr/src/lib/dbunit/sunman/dbunit.3.sunman >> - What process was used to decide that the Interface Stability is >> "uncommitted"? A lot of these decisions are opaque to us, there's no >> Arc Case, not public discussion. Probably more of a question for Jim >> really >> > Referred to SUNWjunit package, so I set it as "Uncommitted". > >> usr/src/pkgdefs/SUNWdbunit/Makefile >> - The line >> >> DATAFILES= depend >> >> is no longer needed now that you are supplying your own depend file. >> >> usr/src/pkgdefs/SUNWdbunit/depend >> - doesn't dbunit have a dependency on Ant and Java? >> - TCL and TK? Did the check_deps throw these up as dependencies? It >> wouldn't surprise me, just checking >> > I added the depend of SUNWj5rt, SUNWj5dev and SUNWjunit. Deleted > SUNWTcl, SUNWTk and SUNWlibmsr >> usr/src/pkgdefs/SUNWdbunit/prototype_com >> - Do all the Java *.class files under usr/share/doc/dbunit/src/test >> exist in a jar file somewhere else in the code you are integrating? >> src folders usually contain just source files but I guess that ant is >> building them in place. There's a lot of them and I think it would be >> better if they weren't there. >> > Fixed > >> Thanks >> >> Amanda >
