Yes, The new webrev url is http://cr.opensolaris.org/~vivekrt/6816397-antlr277
~Vivek R. T. -----Original Message----- From: Paul Cunningham [mailto:[email protected]] Sent: Thursday, March 12, 2009 6:55 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "antlr277" Have you got a new webrev for this so I can see what you have done - the old link does seem to work any more. Paul Vivek Titarmare wrote: > Hi Paul, > > Pl. see my comments INLINE. > > Thanks & Best Regards, > ~Vivek R. T. > > -----Original Message----- > From: Paul Cunningham [mailto:paul.cunningham at tadpole.com] > 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
