Hi Paul, Thanks for the comments.
I have updated the webrev with changes as per your comments. Response inlined Webrev: http://cr.opensolaris.org/~bkumarm/imperius/ Paul Cunningham wrote: > === Start of Comments ==== > > 1. usr/src/lib/Makefile > Add your stuff alphabetically into the list > Done > 2. usr/src/lib/imperius/METADATA > The line ... > 2 PROGRAM: release01-feb08-2009 > should probably be ... > PROGRAM: imperius > Done > Is this the official community release version number? ... > VERSION: 1.0.0 > It is a tagged version. As you suggested in another comment, now have changed the version to " release01-feb08-2009 " > Why has this not got the tarball zip file name at the end > of it ... > 7 SRC: > http://svn.apache.org/viewvc/incubator/imperius/tags/release01-feb08-2009 > Did you zip it up - if so why didn't you use 'tar' and > 'bzip2' (smaller)? > Yes I had zipped it. Now, changed the tarball to tar.bzip2. > 3. usr/src/lib/imperius/Makefile.sfw > Line ... > 29 TARBALL=$(COMPONENT_NAME:sh).zip > I can now see why your METADATA PROGRAM: line is as it > is, but it might be better if the 'release01-feb08-2009' > was on the VERSION: line and you used COMPONENT_VERSION > here instead (less confusing) > Done > Why is this ... > 36 JAVA_ROOT=/usr/jdk/j2sdk1.4.2_06 > different to the JAVA_ROOT in Makefile.master ? > Since Imperius builds only with JDK.1.4.2, I have had to explicitly set the path > The following ... > 29 TARBALL=$(COMPONENT_NAME:sh).zip > 30 SRCDIR=$(COMPONENT_NAME:sh) > could be ... > SRCDIR=$(COMPONENT_NAME:sh) > TARBALL=$(SRCDIR).zip > Done > 4. CDDL HEADER and top of files > You may want to change all the files so that they conform > to that in ... > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > > (see http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines) > In another review (httping), Jim had specifically asked for the removal the extra blank line. Hence followed the same. > 5. usr/src/lib/imperius/imperius.3 > Why duplicate the NAME field text .. > "Is a simple standards based object-oriented policy language > that allows expression of management policies using > condition-action rules." > in both the NAME and DESCRIPTION fields; you probably only > need it in the DESCRIPTION field. > > COPYRIGHT field, > should this have the actual licence name here rather > than saying its 'open-source software' > Done > 6. usr/src/lib/imperius/antlr-2.7.7.jar > Where is this used ? > antlr-2.7.7.jar is used to build the imperius-splcore grammar files. This is required only at build time. > 7. usr/src/pkgdefs/SUNWImperius/copyright > Add the pkg originator copyrights at the top? See > http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright > > Checked a few packages having Apache license (SUNWant, SUNWapch22u, SUNWapch22r) and followed the same http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWant/copyright http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWapch22u/copyright http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWapch22r/copyright > 8. usr/src/pkgdefs/SUNWImperius/depend > Move the Copyright lines to after the > 'CDDL HEADER END' header > Done > 9. usr/src/pkgdefs/SUNWImperius/pkginfo.tmpl > It might be better if you swapped the NAME and DESC text, > eg. to ... > NAME="Imperius" > DESC="Imperius is a simple standards based object-oriented policy > language (1.0.0)" > Done Thanks Bharath
