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/copyright"; > > > > 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 =====
