Spec URL: http://people.redhat.com/sundaram/dbench.spec SRPM URL: http://people.redhat.com/sundaram/dbench-3.04-1.src.rpm Description: Dbench is a filesystem load benchmarking tool originally created by Samba developer and used by OLPC. This has been split off from olpc-utils as suggested in the review of that package.
This is not an official review as I am not sponsored yet. --------- Summary: --------- * rpmlint output not silent * license file not included in RPM * no %doc defined in %files * Changelog missing version information * Files section needs work. * Create a devel package with correct files. Not in the main package * Use macros for %build and %install sections --------- Details: --------- OK - Mock : Built on F-7 (x86) OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. FIX - Spec has consistant macro usage. FIX - Meets Packaging Guidelines. OK - License field in spec matches OK - License is GPL OK - License match packaging policy licenses allowed FIX - License file is included in package License should be included as %doc in the %files section OK - Spec in American English OK - Spec is legible. OK - Sources SHOULD match upstream md5sum: efd0c958da79c1cd941ecd3f63e637ae dbench-3.04.tar.gz OK - Package has correct buildroot. OK - BuildRequires are not redundant. FIX - %build and %install stages are correct and work. why not use, for %prep: %setup -q This would eliminate the need for the pushd and popd For %install why not use: make install DESTDIR=$RPM_BUILD_ROOT It would simplify the %install to 1 line (besides the rm -rf $RPM_BUILD_ROOT), not to mention it would only install the files that are required for the main RPM needed to work OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package is code or permissible content. FIX - Packages %doc files don't affect runtime. There is no %doc section OK - No large doc files not in a -doc package FIX - Package has no duplicate files in %files. %files section needs work OK - Package doesn't own any directories that other packages own. FIX - Changelog section is correct. NA - Does not contain any .la libtool archives NA - .desktop file installed correctly FIX - Should function as described. Binary installs and will run, but package installs binary to /usr/share/dbench which isn't a typical place for a binary thus not in $PATH OK - Should package latest version --------------- Rpmlint output: --------------- SRPM W: dbench no-url-tag W: dbench mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 6) RPM W: dbench no-documentation W: dbench devel-file-in-non-devel-package /usr/share/dbench/io.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/snprintf.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/child.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/system.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/proto.h W: dbench devel-file-in-non-devel-package /usr/share/dbench/config.h W: dbench devel-file-in-non-devel-package /usr/share/dbench/util.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/sockio.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/fileio.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/tbench_srv.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/socklib.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/dbench.c W: dbench devel-file-in-non-devel-package /usr/share/dbench/dbench.h W: dbench no-version-in-last-changelog W: dbench no-url-tag E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/fileio.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/dbench W: dbench unstripped-binary-or-object /usr/share/dbench/dbench E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/tbench_srv W: dbench unstripped-binary-or-object /usr/share/dbench/tbench_srv E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/system.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/snprintf.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/util.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/child.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/tbench W: dbench unstripped-binary-or-object /usr/share/dbench/tbench E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/tbench_srv.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/sockio.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/socklib.o E: dbench arch-dependent-file-in-usr-share /usr/share/dbench/dbench.o
before starting review, I think its better to ask upstream to correct Makefile so that we can just use make install DESTDIR=%{buildroot} and it will install only related files. Current SPEC is wrong its installing all compiled files in parent directory to /usr/share. SHOULD:- Ask upstream to fix Makefile to use buildroot.
The rpm macros are not used (like %setup). Any reason why it is so? Maybe you could have a look at the spectemplate in /etc/rpmdevtools/spectemplate-minimal.spec In the same vein, the pushd and popd are unuseful in %build. The install section is wrong, as Parag said. Maybe you could also have a quick look at the FHS http://www.pathname.com/fhs/ Instead of using DESTDIR (which would be more correct but requires interaction with upstream), you could also use %makeinstall (although this is discouraged, it workaround broken makefile here). To keep timestamps, you could use: %makeinstall INSTALLCMD='install -c -p' Also it seems to me that it would be better to patch Makefile.in such that * -DDATADIR=\"$(datadir)\" becomes -DDATADIR=\"$(datadir)/dbench\" * clinet.txt is installed in $(datadir)/dbench * man pages are installed in $(mandir)/man1 Missing something like %doc README COPYING
Rahul is already sponsored
http://people.freedesktop.org/~johnp/dbench-3.04-1.src.rpm http://people.freedesktop.org/~johnp/dbench.spec I forgot to up the release. There are still cleanups to do but I have to run. This fixes up the most erronous bits. Rahul can you finish this up. Thanks.
What needs to be done here? Is this waiting on a reviewier, or for Rahul to make more changes to the package?
I am ready to continue the review but Comment #5 seemed to imply a new srpm from Rahul.
I will get to it tomorrow guys. I have been struck with other unrelated project work. Same for olpc-logos. Thanks for the patience.
Spec: http://dev.laptop.org/pub/sugar/rpms/dbench.spec SRPM: http://dev.laptop.org/pub/sugar/rpms/dbench-3.04-2.src.rpm New rpms - patch added to have the Makefile.in install the client.txt to the correct location and have dbench.c look for the file in the correct location.
The man pages location is wrong. You can patch Makefile.in or use something along make install DESTDIR=$RPM_BUILD_ROOT mandir=%{_mandir}/man1 Also the timestamps of the man page and client.txt file are not kept. This may be achieved with make install DESTDIR=$RPM_BUILD_ROOT INSTALLCMD='install -p' If you use both tricks, this gives: make install DESTDIR=$RPM_BUILD_ROOT mandir=%{_mandir}/man1 INSTALLCMD='install -p' It seems to me that the leading spaces in %description are not usefull. What is the reason for an explicit BuildRequires: kernel-headers I was under the impression that it was wrong to rely on this package, instead of using the libc headers.
Hi Patrice, http://sundaram.fedorapeople.org/dbench.spec http://sundaram.fedorapeople.org/dbench-3.04-3.src.rpm
Rahul, As mentioned in comment #5, it looks %doc is not added to SPEC. SHOULD: add doc files as %doc README COPYING
Parag, Fixed the docs http://sundaram.fedorapeople.org/dbench.spec http://sundaram.fedorapeople.org/dbench-3.04-4.src.rpm
Review: + package builds in mock (development i386). + rpmlint is silent for SRPM and for RPM. + source files match upstream. efd0c958da79c1cd941ecd3f63e637ae dbench-3.04.tar.gz + package meets naming and packaging guidelines. + specfile is properly named, is cleanly written + Spec file is written in American English. + Spec file is legible. + dist tag is present. + build root is correct. + license is open source-compatible. + License text is included in package. + %doc is small so no need of -doc subpackage. + BuildRequires are proper. + %clean is present. + package installed properly. + Macro use appears consistent. + Package contains code, not content. + no static libraries. + no .pc files are present. + no -devel subpackage exists. + no .la files. + no translations are available. + Does owns the directories it creates. + no duplicates in %files. + file permissions are appropriate. + no scriptlets are used. + Package -> dbench-3.04-4.fc8 Requires: libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.2) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIBC_2.4) rtld(GNU_HASH) + Not a GUI app. APPROVED.
Using dbench_version instead of a plain %{version} seems strange to me but not a blocker. BuildRequires: glibc-headers is unusefull (and may be harmful if the split change), it should be in every build root. In my opinion this is a blocker. I would modify the %description like this (only a suggestion): Dbench is a file system benchmark that generates load patterns similar to those of the commercial Netbench benchmark. No other comments from me.
(In reply to comment #15) > Using dbench_version instead of a plain %{version} > seems strange to me but not a blocker. > > BuildRequires: glibc-headers > is unusefull (and may be harmful if the split change), > it should be in every build root. In my opinion this is > a blocker. > yes. you are right. Thought its needed as BR. But mock built successfully without this BR. > I would modify the %description like this (only a > suggestion): > > Dbench is a file system benchmark that generates load patterns similar > to those of the commercial Netbench benchmark. > > > No other comments from me. Rahul, Can you please resubmit new SRPM links for review? Or you can do this at initial package import time also.
http://sundaram.fedorapeople.org/dbench.spec http://sundaram.fedorapeople.org/dbench-3.04-5.src.rpm Here you go. Fixed version macro. This was just a left over from before this package was split from olpc-utils. Dropped BR on glibc-headers. The description is left as it was since it makes abstract metadata searching easier and matches the OpenSUSE spec description.
For me it looks ok now. APPROVED.
%define %version 3.04 at the beginning is wrong. Remove it and it is right for me. You can do it post-commit.
http://sundaram.fedorapeople.org/dbench.spec http://sundaram.fedorapeople.org/dbench-3.04-6.src.rpm Drop the dumb redundant version info.
Also approved for me.
New Package CVS Request ======================= Package Name: dbench Short Description: Filesystem load benchmarking tool Owners: sundaram,johnp Branches: FC-6 F-7 EL-4 EL-5 OLPC-2. InitialCC: sundaram,johnp Cvsextras Commits: yes
cvs done.
Is this still waiting for any builds or we can CLOSE this review ticket?
Have pushed an update. Closing