Bug 226316

The privoxy spec is borrowed from upstream with many legacy conventions that it
makes it very hard to read and possibly maintain.

Some examples:
- There are some lines that remove carriage returns from configuration files,
although no such action is necessary with the last privoxy version (3.0.6).
Moreover they are really hard to parse.  Example:

for i in `ls *.action`
do
       cat $i | sed -e 's/[[:cntrl:]]*$//' > %{buildroot}%{privoxyconf}/$i
done

- There are many lines dealing with previous names of the package, namely
junkbuster and/or junkbust.  The rename happened about 6-7 years ago, so I think
it is safe to drop this legacy compatibility stuff.

- There are too many consistency checks, such as:
  * consistency between versions in specfile and configure file.
  * consistency between user/group id in the system and user/group id specified
   in the specfile
The privoxy package has remained in this state for the last 6-7 years, so I
suppose it is safe to drop these consistency checks.

- As for the used uid/gid pair, I asked in the fedora-packaging list about the
hardcoded paths, and the answer was that it is ok to keep using them as long as
they are documented in /usr/share/doc/setup-*/uidgid.  Privoxy is (documented),
so it is ok to hardcoded uid,gid=73,73.

- There are some comments in the top of the spec file that only clutter the
readability.

- Buildroot is not according to the fedora guidelines

Since I was some time ago involved in upstream, I tried to clean up the spec
file and follow as close as possible the packaging guidelines.

Specfile and SRPM can be found at:
http://gallagher.di.uoa.gr/any/rpms/privoxy/privoxy.spec
http://gallagher.di.uoa.gr/any/rpms/privoxy/privoxy-3.0.6-3.src.rpm

There are some other issues in the spec file, such as disabling dynamic pcre and
using a static copy, but I did not touch them to keep the functionality as close
to current as possible.

Updated to an actually usable spec:

http://gallagher.di.uoa.gr/any/rpms/privoxy/privoxy.spec
http://gallagher.di.uoa.gr/any/rpms/privoxy/privoxy-3.0.6-4.src.rpm

* Tue Feb 06 2007 Sarantis Paskalis <paskalis.gr> 3.0.6-4
- remove unnecessary perl invocation
- fix Requires(pre), (post), (preun) and (postun) for scriptlets
- fix rpmlint 'conffile-marked-as-executable'
- fix other stuff, so that it can actually be installed and erased
- do not remove user/group on erase because due to logs remaining

Mark the package as reviewed.

I asked about the --disabled-dynamic-pcre in privoxy development list.  There is
an issue in privoxy<=3.0.6, fixed since in CVS, that can trigger a crash.  That
is however a relatively new issue, not an issue since before 2002, when this
switch was added.  Moreover, Debian uses the dynamic lib, so I propose to drop
the swith and to add a Requires: pcre.

Discussion on privoxy devel list is here:
http://thread.gmane.org/gmane.comp.web.privoxy.devel/8000

The patch discussed for privoxy is here:
https://sourceforge.net/tracker/index.php?func=detail&aid=1621173&group_id=11118&atid=111118

most fixes applied, the spec file looks a lot saner now. Thanks !

Karsten,

Some (rather minor compared to the initial spec) issues/questions before approval:

- Why do you convert the man page from ISO-8859-1 to UTF-8?  AFAIK, ISO-8859-1
is a subset of UTF-8, and any ISO-8859-1 text should also be UTF-8 text. 
Moreover, the manpage is in ASCII, an even smaller subset of UTF-8.  Is there
any reason for this conversion?

- The specfile has really changed a lot since the junkbuster days.  Is it really
necessary to keep the first 27 lines stating copyrights of the program and a
summary of the GPL.  I believe a reference of the GPL in the license field
should be enough to make it clear that the program is GPL.

- Regarding the dynamic or static pcre linking, I am not sure what would be the
best thing to do right now.  Fact is that the included pcre library used for
static linking is very old.  It is actually pcre-3.4 dated August 2000!  Any
fixes during the last 6.5 years are ignored by privoxy if it is compiled using
the very old internal copy.  On the other hand, the vast majority of privoxy
users (most privoxy packages are built with --disable-dynamic-pcre, including
Windows builds) use the static internal version, so more testers.  My proposal
would be to switch to dynamic pcre linking as soon as possible even if it causes
some yet unseen bugs.

Other than those issues, the package is good to go.

Another thing:  Please consider adding %{?dist} in Release to make things easier
for updates between different releases.

(In reply to comment #6)
> any ISO-8859-1 text should also be UTF-8 text. 

That's incorrect when thinking about bytes (which is the case here); the
encodings are the same only for the first 128 characters (ie. ASCII).  Beyond
that, they differ, for example many common non-ASCII ISO-8859-1 characters are
encoded as two bytes in UTF-8.  http://en.wikipedia.org/wiki/UTF-8

> Moreover, the manpage is in ASCII, an even smaller subset of UTF-8.  Is there
> any reason for this conversion?

This, however seems to be the case, and no conversion should be necessary.

(In reply to comment #8)
> (In reply to comment #6)
> > any ISO-8859-1 text should also be UTF-8 text. 
> 
> That's incorrect when thinking about bytes (which is the case here); the
> encodings are the same only for the first 128 characters (ie. ASCII).  Beyond
> that, they differ, for example many common non-ASCII ISO-8859-1 characters are
> encoded as two bytes in UTF-8.  http://en.wikipedia.org/wiki/UTF-8

Duh!  I thought I checked it, but mixed again characters with encodings.  Sorry
about that.

> > Moreover, the manpage is in ASCII, an even smaller subset of UTF-8.  Is there
> > any reason for this conversion?
> 
> This, however seems to be the case, and no conversion should be necessary.

I should stick to that only.

I didn't add the man page conversion, that was added by Miloslav Trmac back in
2004. Whatever the reason was, it seems to be gone with privoxy-3.0.6.

-5 has the following fixes:
- add disttag
- don't convert manpage to UTF-8
- use dynamic pcre
- drop license text from spec file, it's already covered in %%doc

OK, all of the issues I saw were addressed with one exception:

- The crash that can be seen with a dynamically linked pcre.  I have reformatted
the patch mentioned in comment #4 to apply in virgin 3.0.6 sources and attach it
here.  For reference see also 
https://sourceforge.net/tracker/index.php?func=detail&aid=1621173&group_id=11118&atid=111118
http://bugs.debian.org/404284
http://ijbswa.cvs.sourceforge.net/ijbswa/current/filters.c?r1=1.72&r2=1.73

Other things (informational only):

rpmlint is not silent but the things it complains about can be ignored
e.g. dozens of 
E: privoxy non-standard-uid /etc/privoxy privoxy
E: privoxy non-standard-gid /etc/privoxy privoxy

and a warning  
W: privoxy incoherent-subsys /etc/rc.d/init.d/privoxy $PRIVOXY_PRG
which can be ignored since the variable $PRIVOXY_PRG is defined to privoxy in
the init file.

Open bugs of the package are the bugs #193159, #198402, and #205011.  The first
is an init file issue replacing "kill -HUP" with "kill -s HUP" (I can't
reproduce the problem locally), the second is an (ongoing?) effort to add IPv6
support and the last one is a SELinux issue about writing in /etc/privoxy.

All of those are considered normal or low priority.

So, if the patch fixing dynamic pcre issues is applied, this package is approved.

Created attachment 148855 [details]
Proposed patch from upstream to work with current dynamic pcre.

dynamic pcre patch and a patch for bug #193159 added to privoxy-3.0.6-6.fc7

All of the issues I mentioned are fixed. 

There are some minor issues I saw at the last check.  The lines in the spec

%dir %{privoxyconf}
%dir %{privoxyconf}/templates
are redundant, since the following covers them:
%config(noreplace) %{privoxyconf}

A warning in rpmlint running the SRPM reveals an unescaped %files in the
changelog dating back in 2002.

Since those issues are not that important, this package is APPROVED.  Please
also fix the aforementioned minor issues at your convenience.

Since all issues are fixed in the latest version, please close this bug rawhide
per the new process at
https://www.redhat.com/archives/fedora-extras-list/2007-February/msg00380.html

(I think that the owner should close this, that is why I don't close it myself
as a reviewer).

Thanks