Forgot to upload the new webrev incorporating Roland's suggestions. It is here: http://cr.opensolaris.org/~venkytv/privoxy-20080828/
Thanks, Venky. On Wed, Aug 20, 2008 at 02:41:07PM +0200, Roland Mainz wrote: > Venky wrote: > > > > I'm porting Privoxy[1] to SFW. Would appreciate a code review of my > > changes. This is my first SFW integration, so it is possible there > > are some beginner's mistakes. > > > > The webrev is here: > > http://cr.opensolaris.org/~venkytv/privoxy/ > > 1. AFAIK these things may be nice (most of these item are very generic > things which are "usual suspects" for build problems in the tree): > - Plese 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) > - Please use either $(SHELL) or /usr/bin/bash for "configure" calls (so > we know which one is used and "configure" doesn't pick one itself) > - Please use /usr/bin/ksh93 or /usr/bin/bash for install-sfw* and add a > $ set -o xtrace # 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) > > 2. Non-generic issues: > > --- /dev/null Tue Aug 12 07:00:27 2008 > > +++ new/usr/src/cmd/privoxy/Makefile.sfw Tue Aug 12 07:00:27 2008 > > @@ -0,0 +1,69 @@ > [snip] > > +$(VER)/privoxy: $(VER)/GNUmakefile > > + (cd $(VER); \ > > + env \ > > Please use "env - " here... > > > + PATH=$(SFW_PATH) \ > > Please add double-quotes around "PATH=$(SFW_PATH)". > > > + LD_OPTIONS="-M $(SRC)/cmd/mapfile_noexstk" \ > > + MAKE=$(GMAKE) \ > > Double-quote, please... > > > + $(GMAKE)) > > + > > +test: > > + > > +install: $(VER)/privoxy > > + VERS=$(VER) /usr/bin/ksh ./install-privoxy > [snip] > > --- /dev/null Tue Aug 12 07:00:31 2008 > > +++ new/usr/src/cmd/privoxy/install-privoxy Tue Aug 12 07:00:31 2008 > > @@ -0,0 +1,109 @@ > > +#!/usr/bin/ksh -pe > > - This line is a no-op since the script is called via $ <interpreter> > <scriptname> # and not $ ./<scriptname> #. IMO you want to add an > explicit $ set -o xtrace # directly under the license template > - Please use "function foo" instead of "foo()" (see > http://www.opensolaris.org/os/project/shell/shellstyle/#use_ksh_style_function_syntax) > - Please make sure the script doesn't generate errors with $ ksh93 -n > <scriptname> # (this is the same as > http://www.opensolaris.org/os/project/shell/shellstyle/#shell_lint) > > ---- > > Bye, > Roland > > -- > __ . . __ > (o.\ \/ /.o) roland.mainz at nrubsig.org > \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer > /O /==\ O\ TEL +49 641 3992797 > (;O/ \/ \O;) -- One hundred thousand lemmings can't be wrong.
