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@xxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lxtnow@xxxxxxxxx ------- Additional Comments From lxtnow@xxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review