[Bug 230275] Review Request: varnish - High-performance HTTP accelerator

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: varnish - High-performance HTTP accelerator


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=230275


bugzilla at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |medium




------- Additional Comments From ingvar at linpro.no  2007-04-17 14:59 EST -------
The short story:

New version of the specfile:
http://users.linpro.no/ingvar/varnish/fedora-extras-commit/varnish.spec
New source rpm:
http://users.linpro.no/ingvar/varnish/fedora-extras-commit/varnish-1.0.3-5.src.rpm

The details:

* Kevin Fenzi
> 1. You might want to use the %{?dist} tag.

OK, added the dist tag in Relase

> 2. You should not strip the binaries and libraries...
> that makes the rpm debuginfo
> package empty. Remove the 'strip -p' lines in install.
> rpmlint says:
> E: varnish-debuginfo empty-debuginfo-package

Ah, I had a %debug_package %{nil} in my .rpmmacros, som I didn't catch
this one with rpmlint. Fixed.

> 3. Don't use $RPM_BUILD_ROOT and %{buildroot}.

OK, fixed.

> 4. You might use for the Source0 url something like:
> http://downloads.sourceforge.net/varnish/varnish-%{version}.tar.gz
> Not a blocker, but nice to not point to a specific mirror.

OK, fixed.

> 5. You shouldn't need to specify Buildroot for each of the
> subpackages. It should pick that up from the top. I think the
> comment above was about "BuildRequires", not "Buildroot". Also, no
> need for another URL tag for the subpackages.

OK, fixed

> 6. Do you need to ship static libs? They are discouraged in Fedora
> for a variety of reasons. Do you know anything that links to just
> the static libs?  Or can we remove them for now and readd a -static
> subpackage later if someone requests?

Yep, removed.

> 7.  Subpackages shouldn't also need to duplicate all the doc files:
> %doc INSTALL LICENSE README ChangeLog redhat/README.redhat

OK, added only the LICENCE file to avoid the "no documentation"
warning from rpmlint.

> Perhaps they should just be in the main package since the others require it?

Yep, that sounds reasonable

> Also, no need to include the INSTALL file, since people reading here will be
> installing via the rpm. ;)

That is not necessarily true. The INSTALL file may contain information
which could inspire users to compile a version of varnish on their
own. Which is not a bad thing. I kept the INSTALL file.

> 8. Does the description in the main package need to have all the
> links and copyright info? 

It's just a dump of the README, so no problem in trimming it down a
bit.

> 9. Should remove the
> /sbin/chkconfig --list varnish
> in %post. rpms shouldn't have any output when installing.  Also
> missing some requires for the init script handling, suggest:
>
> Requires(post): /sbin/chkconfig
> Requires(preun): /sbin/chkconfig
> Requires(preun): /sbin/service

OK, fixed as proposed.

> 10. You need to own the /etc/varnish directory...
> %dir %{_sysconfdir}/varnish/

OK, fixed



-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.