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 ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
