Paul, Thanks for your comments. My responses are inline below
Paul Cunningham wrote: > See below for my comments from my quick skip through ... > > Paul > > On Tue, 2008-07-15 at 22:05, William Schoofs wrote: > > I have posted a webrev for the package "libmikmod" which I am porting to > > OpenSolaris and would like to request a code review. > > > > Please see http://cr.opensolaris.org/~wjs/libmikmod/webrev/ and provide any > > comments as needed if there are any issues which I need to correct. > > ==== Start of comments ==== > > 1. usr/src/pkgdefs/Makefile > You need to resysnc this with the gate so it doesn't > look like you are trying to delete stuff. Done. > > 2. usr/src/pkgdefs/SUNWlibmikmod/depend > Have you run the pkg dependencies script on your > package? Yes > > Move the 'Copyright' lines to after the > 'CDDL HEADER END' header Done. > > 3. usr/src/pkgdefs/SUNWlibmikmod/prototype_sparc > You have the line ... > 'd none usr/bin/sparcv9 755 root bin' > in it twice. > > And same for usr/src/pkgdefs/SUNWlibmikmod/prototype_i386 Each directory line in the files is unique. One is for a lib subdirectory, the other is for a bin subdirectory. d none usr/lib/sparcv9 755 root bin d none usr/bin/sparcv9 755 root bin > > 4. usr/src/lib/libmikmod/METADATA > Add line for URL to where you got source code > tarball from. Done. > > 5. usr/src/lib/libmikmod/Makefile.sfw > Change so you use the predefined '--prefix=' from > Makefile.master, eg. something like .. > CONFIGURE_OPTIONS += --enable-shared > CONFIGURE_OPTIONS += --disable-static > ..... > ./configure $(CONFIGURE_OPTIONS) > There should be various examples of this in the > gate now, eg. cups. > > Combine the two '-rm -rf ...' lines into one. > > Rather than 'all: real-all' & 'real-all: all32 all64' > why not just .. > 'all: all32 all64' Done. > > 5. usr/src/lib/libmikmod/install-libmikmod > Do you need 'chmod 644 ${INFODIR}/dir'? Shouldn't it > be in Targetdirs with the correct permissions? Although the ${INFODIR} directory is in Targetdirs, the ${INFODIR}/dir file is not in Targetdirs. > > 6. usr/src/lib/libmikmod/libmikmod-3.2.0-beta2.tar.gz > Beta version? Is it okay to use this. This is stable software which hasn't changed since 2004. > > 7. Everything else looks okay to me. > > ==== End of comments ====== ---------------------------------------------------------------- Bill Schoofs - Software Engineer Archive Software Software Division, Sun Microsystems Eagan, MN work: (651)554-1552 Ext: 54452 email: William.Schoofs at Sun.COM fax: (651)554-1540 web: www.sun.com ----------------------------------------------------------------
