Paul: Thanks very much for your review. Please see my replies.
Paul Cunningham wrote: > Hi, > > See below for my comments .... > > Paul > > Li Fan wrote: >> >> Thanks for your comment. I've change the Makelife.sfw and re-generate >> webrev. >> >> http://cr.opensolaris.org/~fan.li/aalib > >>> >>> Li Fan wrote: >>>> >>>> I will integrate AA-lib 1.4rc5 into Opensoalris. The webrev is at: > > === Start of Comments ==== > > 1. usr/src/Targetdirs > You do not seem to have changed anything in this > file; please unedit so it doesn't show up in the webrev Done > > 2. usr/src/lib/Makefile > I know the list is not in alphabetical order, but please > put your entry 'aalib' at or near the top of the list (rather > than at the bottom). Done > > 3. usr/src/lib/aalib/METADATA > Make the NAME: field text more descriptive Done > > You can leave the COMMENT: field blank Done > > Shouldn't the URL: point to ... > "http://aa-project.sourceforge.net/aalib/"; This is old website.They have moved to http://sourceforge.net/projects/aa-project > > 4. CDDL HEADER and file top (various files) > Cosmetic: Please change so that this conforms > to that in .. > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > > See http://wikis.sun.com/display/SFWNotes/Package+writing+guidelines > its probably just the missing line space after the > CDDL header (but check). Done > > 5. usr/src/lib/aalib/Makefile.sfw > Explicitly define the 'configure --prefix=...' value using .. > ./configure $(CONFIGURE_OPTIONS) > where CONFIGURE_OPTIONS is defined in Makefile.master Added the configure options. > > Lines ... > 80 -rm -rf $(VER) > 81 -rm -rf $(VER64) > combine into a single line, eg ... > -rm -rf $(VER) $(VER64) Done > > Do you need this line ? ... > 83 install_h: Without this line, run "make setup" in ${SRC} will fail. > > Do you need this line ? ... > 31 MAKE=/usr/bin/make Deleted > > 6. usr/src/lib/aalib/install-sfw > & usr/src/lib/aalib/install-sfw-64 > Remove the write permission bit from files installed > into /usr AA-lib source code default to give write permission to binaries. Fedora aa-lib package also do this. So I think keep as it is would be the best solution. > > 7. usr/src/lib/aalib/install-sfw-64 > Does this work in a script file ? ... > 31 VER=$(COMPONENT_NAME:sh)-$(COMPONENT_VERSION:sh)-64 > You can just pass it in from the Makefile.sfw, eg. > VER=${VER64} Modified. Now install-sfw get VER from Makefile.sfw. Thanks. > > 8. usr/src/pkgdefs/SUNWaalib/copyright > Add the Sun disclaimer and the pkg src owner copyright > lines as in ... > "http://src.opensolaris.org/source/xref/sfw/usr/src/pkgdefs/SUNWmeld/copyright"; > > Added the Sun disclaimer. Thanks. I don't think it's necessary to put author info in copyright, since the GPLv2 license file in aalib source code does not include it, and lots of packages in sfw doesn't have it in copyright file. > > 9. usr/src/pkgdefs/SUNWaalib/pkginfo > This file is generated from your SUNWaalib/pkginfo.tmpl > so should not be in your webrev (remove it) Removed. > > 10. usr/src/pkgdefs/SUNWaalib/prototype_com > & usr/src/pkgdefs/SUNWaalib/prototype_i386 > & usr/src/pkgdefs/SUNWaalib/prototype_sparc > Remove the write permission bit from files installed > into /usr (also see (6)) Same as 6 Thanks ~ Li Fan > > === End of Comments ======
