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

Wed Feb 28 13:21:47 UTC 2007

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

------- Additional Comments From ingvar at linpro.no  2007-02-28 08:21 EST -------
The short story: Fixed according to comments below. New spec/src.rpm :

http://users.linpro.no/ingvar/varnish/fedora-extras-commit/varnish.spec
http://users.linpro.no/ingvar/varnish/fedora-extras-commit/varnish-1.0.3-4.src.rpm

Comments and answers:

> * The packager tag (even commented) SHOULDN'T be in spec file.

ok, removed

> * Buildroot tag SHOULD be
>
http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28Packaging%29#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1

ok, changed to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

> * Some of BuildRequires are redundant as they already installed from
>   development tools and based installed packages (such as gcc,
>   automake, autoconf, patch, libtoll and gcc-c++) all can be drop.

I guess you mean libtool. ok, fixed.

> * Requies: i don't really sure that kernel version can be added as
>   requires especially the kernel 2.6 is already included as default
>   kernel version for FC6 and more.

Kernel version can be added as requires, a least technically, rpm and
rpmbuild allow that. The package should also build on other
distributions. There are people that have tried to build varnish from
a source rpm on RHEL3. The daemon will build, but not work on kernel
<2.6.

> * vendor tag SHOULDN'T be use in spec file.

ok, removed

> * Group tag of %package -n %{lib_name} SHOULD be System
>   Environment/Librairies insteas of System Environment/Daemons.

ok, fixed.

> * Summary tag SHOULD be revisited.
>   (such as %{name} library)

ok, fixed

> * BR and Requires for -libs and -libs-devel packages: same things as
>   above.

You need to specify buildroot for the subpackages as well? ok, fixed.

> * Description -n %{lib_name} SHOULD be revisited (such as,
>   %{lib_name} contains librairies files for %{name})

It's there, but I have trimmed it a bit.

> * Summary tag of %package -n %{lib_name}-devel SHOULD be revisited.
>   (such as development librairies for %{lib_name}.

It's there, but I trimmed it a bit too.

> * Group tag of %package -n %{lib_name}-devel SHOULD be
>   "Development/Librairies instead" instead of "System
>   Environment/Daemons"

ok, fixed

> * url tag in -devel isn't necessary can be remove.

ok, removed

> * -devel Requires is missing and it SHOULDN't be.
>   SHOULD be present : Requires:   %{lib_name} = %{version}-%{release}

Eh? At the moment, it's like this:
Requires: ncurses kernel >= 2.6.0  %{lib_name} = %version-%{release}

> * %description of -devel package SHOULDN be something like :
>   "The %{name}-devel package contains libraries and header files for
>   developing applications that use %{name}." instead.

Well, there is no varnish-devel. Just a varnish-libs-devel, that
contains .a, .la and .so files. So my new description "Development
libraries for %{name}" should be enough, I guess.

> %prep
> * The use of iconv can be improve by using pushd to enter and popd to exit  
>   directories. This fact can reduce the complexity of the iconv command  
>  (including option and redirection command).
> 
> Here is a way that you can use:
> -----------------------------
> pushd bin
> for i in varnishlog varnishtop varnishd \
>   varnishstat varnishhist varnishncsa ; do
>    iconv -f iso-8859-2 -t utf-8 $i/$i.1 > $i/$i.1.utf8
>    rm -f $i/$i.1 && mv $i/$i.1.utf8 $i/$i.1
> done
> popd
> iconv -f iso-8859-2 -t utf-8 man/vcl.7 > man/vcl.7.utf8
> rm -f man/vcl.7 && mv man/vcl.7.utf8 man/vcl.7
> -----------------------------

I may argue that this is less complex, but ok, fixed.

> %build
> *  autogen.sh script SHOULDN'T be use if the configure file is
>    present and work.

Ah, that's a leftover from working with the svn version. Thanks, fixed.

> * From make commande, using %{?_smp_mflags} can improve the
>   procedure and doesn't affect no-smp.

I used to have this, but it killed the build my home computer (maybe
an unstable cpu?). The sources are not large, so compilation time is
not an issue, I think. Now, -jN seems to work on other smp machines,
so ok, added.

> %install
> * the %{makeinstall} should not be used if the "make install
> DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p""
>   functions and it is the case.
> see:
>
http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28Packaging%29#head-fcaf3e6fcbd51194a5d0dbcfbdd2fcb7791dd002

ok, fixed

> * From strip command, you SHOULD comment the use of and add -p option
>   to keep timstamps to files.

ok, fixed

> The use of:
> mkdir -p %{buildroot}%{_docdir}/%{name}-%{version}
> %{__install} -m 0644 INSTALL %{buildroot}%{_docdir}/%{name}-%{version} (...)
> %doc %{_docdir}/%{name}-%{version}/ (...)
>
> SHOULDN'T be used.
> Docs SHOULD be add in %doc in %files section such as "%Doc README
> COPYING ChangeLog".
> It's automaticaly installed in %{_docdir}/%{name}-%{version}.

ok, fixed

> * You don't need to explicit write each files in %files section.
>   use %{_sbindir}/ instead.
>   use %{_mandir}/man1/*.1.gz instead.
>   use %{_mandir}/man7/*.7.gz instead.
>   use %{_libdir}/*.so.* instead.
>   use %{_libdir}/*.so instead.

ok, fixed

> * "*.la" files SHOULDN'T be include in -devel subpackage.
>  if for some reason its really require ( for full work), You should
>  add -statics subpackage.
> 
> Can be remove by using:
> find $RPM_BUILD_ROOT/%{_libdir}/ -name '*.la' -exec rm -f {} ';' 

ok, removed

> * Scriptlets seems to miss some options and arguments. see :
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28script%29

ok, now only service stop and chkconfig --del at uninstall.

> * "/etc" can be replace by its macros %{_sysconfdir}.

ok, fixed

> %changelog
> please add a empty line between each of them.

ok, fixed.

> reflexion : use should think about the naming of -libs and
> -libs-devel subpackag by renaming subpackage as follow, lib%{name}
> and lib%{name}-devel.

I used to do that, but was told by someone that the Fedora/RedHat
standards was name, name-libs, name-devel and/or name-libs-devel. What
is the correct answer? I found no specific information on this at
http://fedoraproject.org/wiki/Packaging/NamingGuidelines.

-- 
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.