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.
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
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
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.
Thanks
Amanda
jin wan - Sun Microsystems - Beijing China wrote:
> Hi Paul & Amanda,
>
> Thanks very much of your review, and please see my answers to both of
> your comments in lines.
>
> I sync the webrev at the same location:
> http://cr.opensolaris.org/~bjwancn/dbunit/
>
> Thanks!
> -James
>
>
>
> 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
>> 3. CDDL HEADER, file top, in various files ..
>> Change so that they conform to those in ...
>> "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/";
>>
>> See ...
>> http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines
>>
> Fixed
>> 4. usr/src/lib/dbunit/Makefile.sfw
>> Change 'env ' to 'env - ' to make sure it only see the
>> env variables they should really get (and not pick-up any
>> random env variable)
>>
>> Extract DBUNIT= info from the METADATA, see recent
>> integrations for examples
>>
>> Use predefined value for ANTHOME=/usr from Makefile.master,
>> eg. ...
>> ANTHOME=$(CFGPREFIX)
>>
> Fixed
>> 5. usr/src/lib/dbunit/install-sfw
>> Pass VER= info in from Makefile.sfw as environment var
>> or an option.
>>
>> Roland Mainz wrote:
>> > 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)
>>
> Fixed
>> 6. usr/src/lib/dbunit/sunman/dbunit.3.sunman
>> Is "Interface Stability External" correct?
>>
> Changed "External" to "Uncommitted"
>> 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";
>
>
>> Change ...
>> "Source for junit is available ..."
>> to
>> "Source for dbunit is available ..."
>>
> Fixed
>> 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
>> 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
>> 9. usr/src/pkgdefs/SUNWdbunit/pkginfo.tmpl
>> Change (for consitancy) ...
>> 42 DESC="dbunit - JUnit database extension 2.3.0"
>> to ..
>> DESC="dbunit - JUnit database extension (2.3.0)"
>>
> 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.
>> 11. usr/src/pkgdefs/SUNWdbunit/prototype_i386
>> & usr/src/pkgdefs/SUNWdbunit/prototype_sparc
>> Cosmetic: add the package name comment line, etc, see ...
>> "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/prototype_i386";
>>
>>
>>
> Fixed
>> === End of Comments ======
>
>
> On 02/20/09 17:21, Amanda waite wrote:
>> usr/src/lib/dbunit/METADATA*
>> * - No need for a CDDL header in the METADATA file
>> - Add a Name field as per the METADATA section of
>> http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines
>> - Add other fields (SRC, SUPPORT, BUGTRAQ)
>>
> Fixed
>> usr/src/lib/dbunit/Makefile.sfw
>> - Set DBUNIT with values from the METADATA file:
>> $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)
>> - In the 'all' target use 'env -' in place of 'env'
>> - In the 'install' target', replace $(SH) with $(SHELL) <- which is
>> ksh93
>>
> Fixed
>> usr/src/cmd/mpg123/install-sfw
>> - add 'set -o errexit' as the first line after the ident string
>> - Pass in 'VER' from Makefile.sfw, i.e.: VER=$(DBUNIT) $(SHELL)
>> ./install-sfw
>>
> Fixed, I got "VER" from METADATA.
>> usr/src/lib/dbunit/sunman/dbunit.3.sunman
>> - Interface Stability is External. Where did this come from? External
>> is now Volatile, is there an ARC case for DBUnit?
>>
> Fixed, see above
>> usr/src/pkgdefs/SUNWdbunit/prototype_com
>> - Just use the package name as the final comment instead of
>> "SUNWdbunit - JUnit database extension"
>>
>> usr/src/pkgdefs/SUNWdbunit/prototype_sparc,
>> usr/src/pkgdefs/SUNWdbunit/prototype_i386
>> - Add the following as the final comments for consistency
>>
>> # source locations relative to the prototype file
>> #
>> #
>> # SUNWdbunit
>> #
>>
> Fixed
>>
>> CDDL headers for all files
>> - Make sure all the CDDL headers and Copyright notices conform to:
>> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
>>
>>
> Fixed
>> Amanda
>
