Hi Vivek,
Below are few comments on the webrev from my quick review:
1. Please check CDDL Header for all the files. It should follow
guidelines specified at below location:
http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
2. In usr/src/lib/antlr277/METADATA
NAME: could be changed to antlr - (AN)other (T)ool for (L)anguage
(R)ecognition
3. For any new directories that you are creating, you need to have an
entry in /usr/src/Targetdirs.
4.* *In usr/src/lib/antlr277/install-sfw
If you are installing only the manpage, install_dir() is not
required.
Thanks
Bharath
Vivek Titarmare wrote:
> Hi,
>
> As mentioned below almost all the points are updated and the new webrev is
> uploaded. The url is http://cr.opensolaris.org/~vivekrt/antlr277.
>
> Couple of points to note is as follows:
>
> 1. Point Number 11 could not be completed as we are working on categories
> and sub-categories for the bugID to be filed. So I have mentioned Pending in
> METADATA file.
>
> 2. Secondly the build.xml is completely removed and the code is tested
> after removing.
>
> Let me know if I have missed anything.
>
> Thanks,
> ~Vivek R. T.
>
> -----Original Message-----
> From: Muktha.Narayan at Sun.COM [mailto:Muktha.Narayan at Sun.COM]
> Sent: Tuesday, March 10, 2009 5:55 PM
> To: Vivek Titarmare
> Cc: sfwnv-discuss at opensolaris.org
> Subject: Re: [sfwnv-discuss] Request code review for "antlr277"
>
> Hi Vivek,
>
> Below are some comments on the antlr webrev at
> http://cr.opensolaris.org/~vivekrt/webrev.
> Also note that some of the comments may be same as what Paul has already
> mentioned.
>
> 1. Targetdirs
> a) Please include the entries in alphabetical order.
>
> 2. METADATA
> a) There are few fields missing in METADATA (NAME, SRC, SUPPORT,
> BUGTRAQ), please follow the rules specified in the link below :
> http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines
>
> b) Also, you should specify your mail id in OWNER section.
>
> 3. usr/src/lib/Makefile
> a) Please include the entries in alphabetical order.
>
> 4. Please check CDDL Header for all the files. It should follow
> guidelines specified at below location:
> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/
>
> 5. In Makefile.sfw,
> a) ANTLR information could be obtained from the METADATA file, instead
> of hard coding it in the Makefile.sfw.
> something like:
> ANTLR= $(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)
>
> b) Please do not use "pragma ident" keywords in files other than source
> files. In install scripts, Makefiles and other files you should be using
> "ident" keyword.
>
> c) Are you using the default Java version ? I don't see any specific env
> being set before building using ant. Is this intended ?
>
> 6. Please make sure the Sun Copyright year is set to 2009 in all files.
>
> 7. In install-sfw,
> a) It is advisable to get the VER info from Makefile.sfw instead of
> specifying it again in install-sfw file.
> Something like below can be done in your Makefile.sfw:
>
> VER=$(ANTLR) $(SHELL) ./install-sfw
>
> b) You could add below lines to make sure the script stops as soon as it
> encounters first error
>
> # stop at first error
> set -o errexit
>
> c) It is not required to specify the permission while installing
> symbolic links
>
> 72 _install L ../../lib/java/javadoc/antlr277 ${DOCDIR}/javadoc 444
>
>
> 8. In the manpage antlr277.3, there is a small typo. A missing space
> (forconstructing)
>
> 9. build.xml
> a) Is there any reason why the default Makefile cannot be used which is
> available in the source tarball.
> b) Also, please check if this file needs to have CDDL header.
>
> 10. prototype_com
> a) shouldn't the below be antlr277.3 ?
>
> 54 f none usr/share/man/man3/antlr.3 0444 root bin
>
> b) Is the below entry required?
>
> 57 d none usr/share/doc/junit 0755 root other
>
> 11. Please make sure that the comments for all the files listed in
> webrev are of the format "<bugid> <synopsis>"
>
> 12. The copyright file should have the source owner copyright lines
> followed by the 'BSD license'.
>
> Regards
> Muktha
>
> Paul Cunningham wrote:
>
>> Vivek,
>>
>> See below for my comments ...
>>
>> Paul
>>
>> Vivek Titarmare wrote:
>>
>>> I have posted a webrev for the package "antlr277" which I am porting
>>> to OpenSolaris and would
>>> like to request a code review.
>>>
>>> Please see http://cr.opensolaris.org/~vivekrt/webrev/
>>> <http://cr.opensolaris.org/%7Eelpineda/git/webrev/> and provide any
>>> comments as
>>> needed if there are any issues which I need to correct.
>>>
>> Have I reviewed the right thing here "antlr277" v "git" ?
>>
>> === Start of Comments ===
>>
>> 1. usr/src/cmd/Makefile
>> I am guess that this is out of sync with the gate as the
>> copyright year is wrong - your ws may need resyncing
>>
>> 2. usr/src/cmd/git/METADATA
>> Please change so that the content conforms to that in ..
>> "http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines";
>>
>> 3. CDDL HEADER and top of files (various files)
>> Cosmetic: Please change the files so that the tops
>> conform to those in ...
>>
>>
> "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/";
>
>> 4. Copyright year (various files)
>> Please correct the year on the file Copyright line
>>
>> 5. usr/src/cmd/git/Makefile.sfw
>> Extract the VER= info from the METADATA, see recent sfw
>> integrations for examples. And also for MANPAGES=
>>
>> Change so that the pkg 'configure' is done a separate
>> rule ($(VER)/config.status: is normal)
>>
>> Change the pkg configure so it is done using the
>> CONFIGURE_OPTIONS method, eg, something like ...
>> ./configure $(CONFIGURE_OPTIONS)
>> as you have no additional options other than those already
>> predefined in the Makefile.master
>>
>> Lines 48-52, could you have done this with a patch file
>> instead on the pkg's Makefile.in prior to the configure?
>>
>> Lines 64 & 71, do you need the 'prefix=$(PREFIX)'
>> on this 'makes' as you have specified it already
>> on the configure?
>>
>> Lines 87 to 89, why the multiply 'rmdir's, couldn't you
>> have just done '/usr/bin/rm -rf tmp' after line 89 ?
>>
>> Line ...
>> 65 @find . -name core -exec rm -f {} \;
>> do you need this, if so why?
>>
>> 6. usr/src/pkgdefs/Makefile
>> It might be better if SUNWgit was moved to after
>> SUNWgimpprint so its more alphabetical
>>
>> 7. usr/src/pkgdefs/SUNWgit/copyright
>> Add the pkg source owner copyright lines after the
>> sun disclaimer, ie. those extracted from the files in
>> the unpacked src tarball, something like this ...
>>
>>
> "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyrig
> ht"
>
>> 8. usr/src/pkgdefs/SUNWgit/prototype_i386
>> & usr/src/pkgdefs/SUNWgit/prototype_sparc
>> Are the permissions correct on the installed files (0400)?
>>
>> === End of Comments =====
>>
>
>
> _______________________________________________
> sfwnv-discuss mailing list
> sfwnv-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/sfwnv-discuss
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090311/bf22fa75/attachment.html>
