Thanks for the quick review, Paul. Responses inline.
On Wed, Aug 13, 2008 at 08:38:37AM +0100, Paul Cunningham wrote:
> Venky,
>
> See below for my comments from my quick skip through ...
>
> Paul
>
> 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/
>>
>> References:
>> [1] Privoxy is a non-caching web proxy with filtering capabilities.
>> http://www.privoxy.org/
>
> ==== Start of Comments ====
>
> 1. usr/src/cmd/privoxy/METADATA
> You need to add all the missing fields - see other
> pkg METADATA files for examples.
Will do. I picked the METADATA sample from the usr/cmd/p7zip.
Guess that's slightly old.
> 2. usr/src/cmd/privoxy/Makefile.sfw
> You could use the default '--prefix=' (line 53) from
> Makefile.master now, eg. configure $(CONFIGURE_OPTIONS).
> See other recent package integrations for examples.
Will do.
> Use the default '/etc' value defined in
> Makefile.master.
>
> Do you need to explicitly configure the var and man
> dirs for clarity? Again using the values from
> Makefile.master
Right.
> Do you need to run 'autoconf', could you have just
> used the 'configure' that came with the src?
Weird. I seem to remember not finding the configure script which is
why I generated it. But the tarball does contain the script. Will
change the Makefile to use it.
> 3. usr/src/cmd/privoxy/Solaris/http-privoxy
> Does this need a 'stop' case for completeness?
I'm right now using the SMF ":kill" method in http-privoxy.xml.
Figured I'd not add a stop method to the script since it'll not be
used by SMF and there should be no other consumers for this script.
> 4. usr/src/cmd/privoxy/install-privoxy
> Is 'LOGDIR=${PREFIX}/var/log/privoxy' correct, eg.
> /usr/var? Is it used, if not remove it.
You're right. It is not required anymore. Will remove it.
> 5. SUNWprivoxyu
> Have you run the dependencies checker script on your
> pkg to ensure it doesn't really have any dependencies?
Actually, I did not know about the dependency checker. I assume you
are referring to the one of these:
http://wikis.sun.com/display/WebStack/DependsScript
http://xserver.sfbay/~alanc/check-deps.pl
Will run these on the prototypes.
> Doesn't it depend on SUNWprivoxyr ?
That's what I initially figured, but the package I used as an
example (SUNWsquid{ru}) did not have dependencies between the root
and usr packages. Guess I have been picking the wrong examples! :)
> 6. Everything else looks okay to me.
Thanks, Paul. Will make the changes and upload a new webrev.
Cheers,
Venky.
--
One hundred thousand lemmings can't be wrong.
