Thanks Bharath, I have done with all the suggested changes.
Let me know if I am missing anything. Best Regards, ~Vivek R. Titarmare From: Bharath.Madakatte at Sun.COM [mailto:[email protected]] Sent: Wednesday, March 11, 2009 8:01 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "antlr277" 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:[email protected]] 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/> <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> "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/> "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> "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyrig ht <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/20090312/9e84e53e/attachment.html>
