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 ===== -- ---------------------------------------------------------------------- Paul Cunningham Software Engineer Tadpole Business Unit
