Bharath, See below for my comments ...
Paul Bharath Kumar wrote: > Requesting a code review for imperius. > > Imperius (Simple Policy Language) or SPL - Is a simple standards based > object-oriented policy language. > > Webrev: http://cr.opensolaris.org/~bkumarm/imperius/ === Start of Comments ==== 1. usr/src/lib/Makefile Add your stuff alphabetically into the list 2. usr/src/lib/imperius/METADATA The line ... 2 PROGRAM: release01-feb08-2009 should probably be ... PROGRAM: imperius Is this the official community release version number? ... VERSION: 1.0.0 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)? 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) Why is this ... 36 JAVA_ROOT=/usr/jdk/j2sdk1.4.2_06 different to the JAVA_ROOT in Makefile.master ? The following ... 29 TARBALL=$(COMPONENT_NAME:sh).zip 30 SRCDIR=$(COMPONENT_NAME:sh) could be ... SRCDIR=$(COMPONENT_NAME:sh) TARBALL=$(SRCDIR).zip 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) 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' 6. usr/src/lib/imperius/antlr-2.7.7.jar Where is this used ? 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 8. usr/src/pkgdefs/SUNWImperius/depend Move the Copyright lines to after the 'CDDL HEADER END' header 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)" === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
