Bug 244936

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