Vivek, I still don't see a "depend" file; shouldn't that file be part of the package?
Also, Paul, the package to review is antlr277 (vs Git - which was integrated last year, and for which you also provided a review - thanks!). Enrique 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 ===== >> > > > -- Enrique Lopez-Pineda Sun Microsystems enrique.lopezpineda at sun.com 651-552-6426 (x44626) -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090311/95bc37a2/attachment.html>
