Bug 244936 - Review Request: dbench - Filesystem load benchmarking tool
Review Request: dbench - Filesystem load benchmarking tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-19 19:06 EDT by Rahul Sundaram
Modified: 2013-03-13 01:42 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-09-26 11:57:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
panemade: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Rahul Sundaram 2007-06-19 19:06:44 EDT
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.
Comment 1 Tyler Owen 2007-06-19 20:52:35 EDT
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

Comment 2 Parag AN(पराग) 2007-06-20 06:03:13 EDT
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.
Comment 3 Patrice Dumas 2007-06-20 06:55:43 EDT
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
Comment 4 John (J5) Palmieri 2007-06-20 16:28:59 EDT
Rahul is already sponsored
Comment 5 John (J5) Palmieri 2007-06-20 17:02:33 EDT
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.
Comment 6 Jason Tibbitts 2007-07-04 15:52:24 EDT
What needs to be done here?  Is this waiting on a reviewier, or for Rahul to
make more changes to the package?
Comment 7 Patrice Dumas 2007-07-04 17:06:09 EDT
I am ready to continue the review but Comment #5 seemed to
imply a new srpm from Rahul.
Comment 8 Rahul Sundaram 2007-07-04 17:55:49 EDT
I will get to it tomorrow guys. I have been struck with other unrelated project
work. Same for olpc-logos. Thanks for the patience. 
Comment 9 John (J5) Palmieri 2007-07-11 15:52:55 EDT
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.
Comment 10 Patrice Dumas 2007-07-12 02:15:36 EDT
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.
Comment 12 Parag AN(पराग) 2007-09-11 01:19:33 EDT
Rahul,
  As mentioned in comment #5, it looks %doc is not added to SPEC.
SHOULD:
add doc files as 
%doc README COPYING
Comment 14 Parag AN(पराग) 2007-09-11 04:38:51 EDT
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.
Comment 15 Patrice Dumas 2007-09-11 04:53:23 EDT
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.
Comment 16 Parag AN(पराग) 2007-09-11 05:17:31 EDT
(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.
Comment 17 Rahul Sundaram 2007-09-11 05:22:53 EDT
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. 
Comment 18 Parag AN(पराग) 2007-09-11 05:28:13 EDT
For me it looks ok now.
APPROVED.
Comment 19 Patrice Dumas 2007-09-11 05:30:58 EDT
%define %version 3.04

at the beginning is wrong. Remove it and it is right for me.
You can do it post-commit.
Comment 20 Rahul Sundaram 2007-09-11 05:43:49 EDT
http://sundaram.fedorapeople.org/dbench.spec
http://sundaram.fedorapeople.org/dbench-3.04-6.src.rpm

Drop the dumb redundant version info. 
Comment 21 Patrice Dumas 2007-09-11 05:48:33 EDT
Also approved for me.
Comment 22 Rahul Sundaram 2007-09-11 06:01:06 EDT
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
Comment 23 Kevin Fenzi 2007-09-11 12:16:19 EDT
cvs done.
Comment 24 Parag AN(पराग) 2007-09-26 07:25:42 EDT
Is this still waiting for any builds or we can CLOSE this review ticket?
Comment 25 Rahul Sundaram 2007-09-26 11:57:31 EDT
Have pushed an update. Closing

Note You need to log in before you can comment on or make changes to this bug.