Hi Paul, Thanks for reviewing it once again. I have uploaded the changes and replaced the data in the webrev url (http://cr.opensolaris.org/~vivekrt/6816397-antlr277).
Also pl. see my comments below INLINE.. Thanks a lot, ~Vivek R. Titarmare -----Original Message----- From: Paul Cunningham [mailto:[email protected]] Sent: Thursday, March 12, 2009 8:09 PM To: Vivek Titarmare Cc: sfwnv-discuss at opensolaris.org Subject: Re: [sfwnv-discuss] Request code review for "antlr277" Vivek, See below ... Paul Vivek Titarmare wrote: > The new webrev url is > http://cr.opensolaris.org/~vivekrt/6816397-antlr277 >> >> Pl. see my comments INLINE. >> >> >> 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. >>>>> I have posted a webrev for the package "antlr277" which I am porting >>>>> to OpenSolaris and would like to request a code review. .... cut ... >> >> 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. > [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) Normally that is the install dir where it will run it from, ie. /usr/.... but you have defined it as $(ROOT)/usr which is the proto area in the build workspace (ws) - the ws proto area is only a temporary install place while you create the SUNW package - so your configure prefix should be ... ./configure --prefix=$(CFGPREFIX) but you could use the CONFIGURE_OPTIONS (defined in Makefile.master) method instead, eg ... ./configure $(CONFIGURE_OPTIONS) as you no longer seem to be installing into /usr/share/lib/antlr2.7.7 [VIVEK1] Done.. > > [VIVEK] c. Moved "$(GMAKE) install" from target "all" to > "install": Is this change correct? well the place is right, but ... Doesn't it need to know to install it (temporarily) into the ws proto area (when you have changed the 'configure --prefix' above). [VIVEK1] done as per above point. >> 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). So is it possible that in the future you, or someone-else at Sun, may need/want to update this to version 2.n.x? If so it might be better if the directory, etc., was just antlr2 (just checking). [VIVEK1] actually no antlr2 series would come. All would come is Antlr 3.x series and my next review would be for antlr 3.0 which would have just the name as antlr. >> 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. It still looks as though you are trying to delete stuff you don't want to (in the webrev page). [VIVEK1] I had to do the complete bringover to resolve this. Now it looks good.. >> >> 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. Okay if you think it is okay (I was just checking) [VIVEK1] :) >> 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? The normal way to do it when you use the pkg's own 'make install' is to set the correct permissions in the SUNWantlr277/prototype_* files and then run the tools/protofix tool on the SUNW pkg. See good example in .. "http://src.opensolaris.org/source/xref/sfw/usr/src/cmd/cups/Makefile.sfw"; [VIVEK1] after using the tool the permissions are changes as expected. Thanks >> 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. okay so we now have /usr/bin/antlr (and antlr-config). Ah, but when antlr 3.0 is integrated will there be another bin/antlr executable? If so what will happen then? [VIVEK1] Yes, a big question. Can we rename the file after install in the /usr/bin folder? At presently that would not be required, but when antlr 3.0 comes in we need a space for the same. However in the case of antlr 3.0, antlr 2.7.7 is a dependency which would have its own jar file of antlr 2.7.7. Replacing the existing antlr file would not matter a lot when antlr 3.0 comes into picture. Speaking about bin/antlr, shouldn't there be a man page for it as your antlr277.3 doesn't mention it. [VIVEK1] manpages are taken care by using different names for both antlr277 and antlr. Final manpages would be antlr for antlr 3.0. Additional comments ... 1. usr/src/Targetdirs This needs resyncing with the gate otherwise it looks as though you are try to delete stuff. [VIVEK1] I had to struggle a bit on this, but now it is done. 2. usr/src/lib/antlr277/Makefile.sfw GMAKE line ... 33 GMAKE=/usr/sfw/bin/gmake is already defined in Makefile.master so you don't need to redefine it [VIVEK1] Removed.. 4. usr/src/pkgdefs/SUNWantlr277/prototype_com You need to add the directory lines for the following .. d none usr d none usr/bin d none usr/share etc. [VIVEK1] added. 3. usr/src/pkgdefs/SUNWantlr277/prototype_com & usr/src/lib/antlr277/Makefile.sfw The SUNW pkg now installs stuff as .. 53 f none usr/bin/.... 56 d none usr/share/antlr-2.7.7 63 d none usr/share/doc/antlr-2.7.7 does your current 'configure' options in Makefile.sfw allow this to happen? ie, does it add on the share/antlr-2.7.7 and doc/antlr-2.7.7 bits when it installs into the proto area. [VIVEk1] yes it added in share/antlr-2.7.7 and share/doc/antlr-2.7.7 in proto area. Everything else looks okay -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
