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@xxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review