Hi Paul, Pl. see my comments INLINE.
Thanks & Best Regards, ~Vivek R. T. -----Original Message----- From: Paul Cunningham [mailto:[email protected]] Sent: Wednesday, March 11, 2009 9:32 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "antlr277" 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 [VIVEK] Done 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) [VIVEK] a. As per INSTALL.txt provided by ANTLR ( antlr-2.7.7.tar.gz ) , --prefix is to be used for specifying INSTALLDIR . The same thing is being used in the Makefile. Do you want us to change that (this may trigger changes in the original files in the tar) [VIVEK] b. TARBALL=$(ANTLR).tar.gz : done [VIVEK] c. Moved "$(GMAKE) install" from target "all" to "install": Is this change correct? 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/"; [VIVEK] Done 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. [VIVEK] Antlr277 is because we are also going to integrate antlr 3.0 which is a part of Drools dependency and antlr 3.0 would actually go in antlr folder as you mentioned. Two names are registered namely SUNWantlr277 and SUNWantlr. Same is the case with manpages which would probably look for antlr277 initially but when antlr comes end users can use only antlr (which would contain all the information) FYI:: antlr 3.0 requires antlr277 and StringTemplate as a dependency. Both packages would come soon for code review (once antlr277 is finalized). 5. usr/src/lib/antlr277/install-sfw The install_dir() function is not used so delete lines 31-52. [VIVEK] Done 6. usr/src/lib/antlr277/sunman/antlr277.3 If you have written this you should probably add the CDDL header and Sun Copyright stuff. [VIVEK] Done. Added CDDL header to antlr277.3 manpage. 7. usr/src/pkgdefs/Makefile You need to resync with the gate otherwise looks as though you are deleting stuff. [VIVEK] Done. "wx co" before editing file. 8. Dependencies You are using the default 'depend', have you checked there are no other dependencies? [VIVEK] No other dependencies so default is 'depend'. 9. usr/src/pkgdefs/SUNWantlr277/copyright Is this the full "BCD License" text, if not you need to include it (I think). [VIVEK] Actually these licenses are copied from the approved OSR which we had put while filing the OSR. BSD license are picked up from antlr. Should we be copying it from anywhere else, let me know. 10. usr/src/pkgdefs/SUNWantlr277/pkginfo.tmpl Put the version number on the DESC= line in brackets, eg... DESC="................ (2.7.7)" [VIVEK] Done 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. [VIVEK] f none usr/bin/antlr 0755 root root f none usr/bin/antlr-config 0755 root root Both the above files are extracted and installed from antlr tar/Makefile file. Nothing is changed by us manually. Do we need to change default permission and then generate the prototype_com file? 12. Install path usr/share/lib/antlr2.7.7/bin Why is this pkg installed hidden away rather than in /usr/bin ? [VIVEK] Changed to /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/prototy pe_i386" [VIVEK] Done. === End of Comments ====== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
