Paul, Thanks for your review comments. I have incorporated all except comment 3.
Thanks, Abhijit On 03/19/09 17:08, Paul Cunningham wrote: > Abhijit, > > See below for my comments from my quick skip through ... > > Paul > > Abhijit Nath wrote: >> >> Please help review the changes to integrate DOSBox v. 0.72 (CR >> 6657028). I have incorporated the basic review comments. >> The webrev is at: http://cr.opensolaris.org/~an230044/dosbox/ > > === Start of Comments === > > 1. usr/src/cmd/dosbox/METADATA > I think you can remove the text on the COMMENTS: ie. just .. > 12 COMMENTS: > > 2. CDDL HEADER and top of files (various files) > Cosmetic: Change the tops of files so that they > conform to that in .. > "http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/"; > > > 3. usr/src/cmd/dosbox/Makefile.sfw > Change as follows .. > Roland Mainz wrote: > > use "env - ..." and not "env ..." in the Makefiles > > to make sure "configure"&&"make" only see the env > > variables they should really get (and not pick-up > > any random env variable). > > use either $(SHELL) or /usr/bin/bash for "configure" > > calls (so we know which one is used and "configure" > > doesn't pick one itself). > > Cosmetc: Add line-space after the line .. > 29 include ../Makefile.cmd > > 4. usr/src/cmd/dosbox/install.sfw > Rename install.sfw to install-sfw its more common name > > Change as follows .. > Roland Mainz wrote: > > use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* > > and add a $ set -o errexit # at the beginning and > > replace ". ${SRC}/tools/install.subr" with > > "source ${SRC}/tools/install.subr" (the idea is to > > catch failures in the script and abort it at that > > point, right now the script will just continue) > > Delete line .. > 52 #echo "******" `pwd` "******" > > Line ... > 45 # The gtar manpages > 'gtar' ??? > > Wouldn't these have already been install by the > 'make install' in your Makefile.sfw ? ... > 42 _install E src/dosbox ${PREFIX}/bin/dosbox 555 > 54 _install M docs/dosbox.1 ${MAN1DIR}/dosbox.1 444 > (see comment (8) though also) > > 5. usr/src/cmd/dosbox/sunman-stability > Is the following state correct ? .. > 22 Interface Stability Volatile > > 6. usr/src/pkgdefs/SUNWdosbox/depend > Move the Copyright lines to after the CDDL HEADER END > header. > > Why ... > 53 P SUNWgccruntime GCC Runtime libraries > when Makefile.sfw is using the Sun compiler to build > it ? Are all the others correct? > > 7. usr/src/pkgdefs/SUNWdosbox/pkginfo.tmpl > Add the version number to the end of the DESC= > line, eg ... > DESC="...... (0.72)" > > 8. usr/src/cmd/dosbox/Makefile.sfw & prototype_com > As you are only delivering two files in your > package which are done in install-sfw, do you need > lines ? ..... > 45 (cd $(VER); env \ > 46 PATH=$(SFW_PATH) \ > 47 CC=$(CC) CXX=$(CXX) \ > 48 "CFLAGS=$(CFLAGS)" \ > 49 MAKE=$(CCSMAKE) \ > 50 DESTDIR=$(ROOT) \ > 51 INSTALL="$(INSTALL) -c" \ > 52 $(CCSMAKE) install) > > 9. usr/src/pkgdefs/SUNWdosbox/prototype_com > I assume this only works on i386, so shouldn't the > files be in SUNWdosbox/prototype_i386 rather than in > here ? > > 10. usr/src/pkgdefs/SUNWdosbox/prototype_i386 > & usr/src/pkgdefs/SUNWdosbox/prototype_sparc > Move the Copyright lines to after the CDDL HEADER END > header. > > Delete the line (at bottom of the files) ... > # information: Portions Copyright [yyyy] [name of copyright owner] > > === End of Comments =====
