Bharath,
Other than the things from yesterday afternoon (UK time) and Mike's
follow on, this looks good to me except for ...
usr/src/pkgdefs/SUNWImperius/pkginfo.tmpl
Put version (what ever that might be) on the DESC= line, eg. ...
DESC=".............. (my-version)"
Paul
Bharath Kumar wrote:
>
> 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
--
----------------------------------------------------------------------
Paul Cunningham
Software Engineer
Tadpole Business Unit
