See below ...
> -----Original Message-----
> From: Abhijit.Nath at Sun.COM [mailto:Abhijit.Nath at Sun.COM]
> Sent: 20 March 2009 08:55
> To: Cunningham, Paul
> Cc: sfwnv-discuss at opensolaris.org
> Subject: Re: [sfwnv-discuss] Code review request: DOSBox v. 0.72
>
> 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).
You should apply this also, and if it causes your build to fail find out
why; as its probably picking up stuff from your environment that may not
be there when someelse builds it and so fail for them.
I'll recheck the other stuff on Monday.
paul
> > 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 =====
>
>
