[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


lxtnow at gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |lxtnow at gmail.com




------- Additional Comments From lxtnow at gmail.com  2007-02-27 23:47 EST -------
After a first look on your spec file:
------

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

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

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

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

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

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

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

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

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

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

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

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

* -devel Requires is missing and it SHOULDN't be.
  SHOULD be present : Requires:   %{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.

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

----
%build
----

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

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

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

* From strip command, you SHOULD comment the use of and add -p option
  to keep timstamps to files.
----
The use of:
----
mkdir -p %{buildroot}%{_docdir}/%{name}-%{version}

and

%{__install} -m 0644 INSTALL %{buildroot}%{_docdir}/%{name}-%{version}/INSTALL
%{__install} -m 0644 LICENSE %{buildroot}%{_docdir}/%{name}-%{version}/LICENSE
%{__install} -m 0644 README %{buildroot}%{_docdir}/%{name}-%{version}/README
%{__install} -m 0644 ChangeLog %{buildroot}%{_docdir}/%{name}-%{version}/ChangeLog
%{__install} -m 0644 redhat/README.redhat
%{buildroot}%{_docdir}/%{name}-%{version}/README.redhat

and

%doc %{_docdir}/%{name}-%{version}/INSTALL
%doc %{_docdir}/%{name}-%{version}/LICENSE
%doc %{_docdir}/%{name}-%{version}/README
%doc %{_docdir}/%{name}-%{version}/README.redhat
%doc %{_docdir}/%{name}-%{version}/ChangeLog

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

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

* "*.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 {} ';' 


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

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

----
%changelog
----

please add a empty line between each of them.

-----

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



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