Vivek, See below for my comments (hopefully for the right thing this time) from my skip through ...
Paul Vivek Titarmare wrote: > > 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. > >> >> 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. === Start of Comments ==== 1. usr/src/lib/antlr277/METADATA Make the NAME: field more descriptive 2. usr/src/lib/antlr277/Makefile.sfw The configure line doesn't look right to me ... 45 ./configure --prefix=${ROOT}/usr/share/lib/antlr2.7.7 the prefix normally tell the configuration where the built executables will run from at runtime, ie. /usr/... but your prefix is saying it will run from the workspace's proto area. So it should probably be .... ./configure --prefix=$(CFGPREFIX)/share/lib/antlr2.7.7 where CFGPREFIX is defined in Makefile.master Similarly the configure line ... 35 $(GMAKE) install doesn't tell it to install in the ws proto area, it probably needs DESTDIR (or whatever) defined. And why are you doing the install in the 'all:' rule, the 'make install' should be done in the 'install:' rule? You could add after the VER= line ... TARBALL=$(ANTLR).tar.gz then replace elsewhere $(ANTLR).tar.gz with $(TARBALL) 3. CDDL HEADER and top of file (check all files) Cosmetic: Make the top of the files conform to that defined in ... "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; 4. antlr277 in workspace pathname Why have you got the version 277 in the pathnames, it should probably be just usr/src/lib/antlr/.... Same also applies to the SUNW package name SUNWantlr277, it should probably just be SUNWantlr And the man page name sunman/antlr277.3 otherwise this will all have to change again for the next revision update from the opensource community. 5. usr/src/lib/antlr277/install-sfw The install_dir() function is not used so delete lines 31-52. 6. usr/src/lib/antlr277/sunman/antlr277.3 If you have written this you should probably add the CDDL header and Sun Copyright stuff. 7. usr/src/pkgdefs/Makefile You need to resync with the gate otherwise looks as though you are deleting stuff. 8. Dependencies You are using the default 'depend', have you checked there are no other dependencies? 9. usr/src/pkgdefs/SUNWantlr277/copyright Is this the full "BCD License" text, if not you need to include it (I think). 10. usr/src/pkgdefs/SUNWantlr277/pkginfo.tmpl Put the version number on the DESC= line in brackets, eg... DESC="................ (2.7.7)" 11. usr/src/pkgdefs/SUNWantlr277/prototype_com Remove the write permission bit on files installed into /usr/.... ie. ... 54 f none usr/share/lib/antlr2.7.7/bin/antlr 0755 root root and any others. 12. Install path usr/share/lib/antlr2.7.7/bin Why is this pkg installed hidden away rather than in /usr/bin ? 13. usr/src/pkgdefs/SUNWantlr277/prototype_i386 & usr/src/pkgdefs/SUNWantlr277/prototype_sparc Cosmetic: add the pkg name on a comment line as in ... "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/prototype_i386"; === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
