Bug 842013 - Review Request: nfsometer - NFS Performance Framework Tool
Review Request: nfsometer - NFS Performance Framework Tool
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Scherer
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-20 17:16 EDT by Steve Dickson
Modified: 2013-11-21 10:39 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
misc: fedora‑review+
limburgher: fedora‑cvs+
steved: rhel‑rawhide?


Attachments (Terms of Use)

  None (edit)
Description Steve Dickson 2012-07-20 17:16:49 EDT
Spec URL: http://steved.fedorapeople.org/nfsometer/nfsometer.spec  
SRPM URL: http://steved.fedorapeople.org/nfsometer/nfsometer-1.1-0.fc17.src.rpm
Description: 
NFSometer is a performance measurement framework for running workloads and
reporting results across NFS protocol versions, NFS options and Linux
NFS client implementations.

Fedora Account System Username: steved
Comment 1 Michael Scherer 2012-07-21 04:14:33 EDT
Seems the file is not readable :

Cannot download file(s): 'Error 403 downloading http://steved.fedorapeople.org/nfsometer/nfsometer-1.1-0.fc17.src.rpm'
Comment 2 Steve Dickson 2012-07-23 09:50:19 EDT
(In reply to comment #1)
> Seems the file is not readable :
> 
> Cannot download file(s): 'Error 403 downloading
> http://steved.fedorapeople.org/nfsometer/nfsometer-1.1-0.fc17.src.rpm'

Please retry... it was an selinux issue on fedorapeople.org...
Comment 3 Michael Scherer 2012-07-24 17:21:53 EDT
Build in mock fail :

Exécution_de(%build): /bin/sh -e /var/tmp/rpm-tmp.E1njxS
+ umask 022
+ cd /builddir/build/BUILD
+ cd nfsometer-1.1
+ LANG=C
+ export LANG
+ unset DISPLAY
+ /usr/bin/python setup.py build
Traceback (most recent call last):
  File "setup.py", line 15, in <module>
    from setuptools.command.install import install as _install
ImportError: No module named setuptools.command.install


I think a BR is missing ( like python2-devel, or python3-devel )

Also, I think the COPYING file should be in %doc, as well as README ( the former for legal reason, the later for usabilty )

As a side note, I usually recommend to have 1 line for each requires, this permit to have IMHO better diff ( ie, +/- show the only change )
Comment 4 Steve Dickson 2012-07-25 09:12:40 EDT
(In reply to comment #3)
> Build in mock fail :
> 
> Exécution_de(%build): /bin/sh -e /var/tmp/rpm-tmp.E1njxS
> + umask 022
> + cd /builddir/build/BUILD
> + cd nfsometer-1.1
> + LANG=C
> + export LANG
> + unset DISPLAY
> + /usr/bin/python setup.py build
> Traceback (most recent call last):
>   File "setup.py", line 15, in <module>
>     from setuptools.command.install import install as _install
> ImportError: No module named setuptools.command.install
> 
> 
> I think a BR is missing ( like python2-devel, or python3-devel )
python-setuptools was missing...

> 
> Also, I think the COPYING file should be in %doc, as well as README ( the
> former for legal reason, the later for usabilty )
Done!

> 
> As a side note, I usually recommend to have 1 line for each requires, this
> permit to have IMHO better diff ( ie, +/- show the only change )
Good idea... 

Here the diff:

diff .old/nfsometer.spec nfsometer.spec 
--- .old/nfsometer.spec	2012-07-20 21:13:39.911420286 +0000
+++ nfsometer.spec	2012-07-25 13:09:22.960228234 +0000
@@ -1,6 +1,6 @@
 Name:    nfsometer		
 Version: 1.1	 
-Release: 0%{?dist}
+Release: 1%{?dist}
 Summary: NFS Performance Framework Tool
 
 Group:   System Tools
@@ -8,7 +8,12 @@ License: GPLv2 
 URL: http://wiki.linux-nfs.org/wiki/index.php/NFSometer
 Source0: http://www.linux-nfs.org/~dros/nfsometer/releases/%{name}-%{version}.tar.gz 
 
-Requires: nfs-utils python-matplotlib numpy python-mako filebench
+BuildRequires: python-setuptools
+Requires: nfs-utils 
+Requires: python-matplotlib
+Requires: numpy 
+Requires: python-mako
+Requires: filebench
 
 %description
 NFSometer is a performance measurement framework for running workloads and 
@@ -30,8 +35,11 @@ NFS client implementations. 
 #For noarch packages: sitelib
 %{python_sitelib}/*
 
-%doc
+%doc COPYING README
 
 %changelog
+* Wed Jul 25 2012 Steve Dickson <steved@redhat.com> 1.1-1
+- Incorporated review comments.
+
 * Thu Jul 19 2012 Steve Dickson <steved@redhat.com> 1.1-0
 - Inital commit.

http://steved.fedorapeople.org/nfsometer/ has been updated
Comment 5 Michael Scherer 2012-07-25 16:28:21 EDT
+ /usr/bin/python setup.py build
Error importing numpy - Make sure numpy is installed

seems numpy is missing :)

In fact to make it build, I have added the following ( setup.py check runtime deps at build time ) :

--- /home/misc/checkout/git/FedoraReview/nfsometer/nfsometer.spec	2012-07-25 15:09:22.000000000 +0200
+++ SPECS/nfsometer.spec	2012-07-25 22:24:33.383177536 +0200
@@ -9,6 +9,9 @@
 Source0: http://www.linux-nfs.org/~dros/nfsometer/releases/%{name}-%{version}.tar.gz 
 
 BuildRequires: python-setuptools
+BuildRequires: numpy
+BuildRequires: python-matplotlib
+BuildRequires: python-mako
 Requires: nfs-utils 
 Requires: python-matplotlib
 Requires: numpy
Comment 6 Steve Dickson 2012-07-26 09:40:04 EDT
(In reply to comment #5)
> + /usr/bin/python setup.py build
> Error importing numpy - Make sure numpy is installed
> 
> seems numpy is missing :)
> 
> In fact to make it build, I have added the following ( setup.py check
> runtime deps at build time ) :
> 
> --- /home/misc/checkout/git/FedoraReview/nfsometer/nfsometer.spec	2012-07-25
> 15:09:22.000000000 +0200
> +++ SPECS/nfsometer.spec	2012-07-25 22:24:33.383177536 +0200
> @@ -9,6 +9,9 @@
>  Source0:
> http://www.linux-nfs.org/~dros/nfsometer/releases/%{name}-%{version}.tar.gz 
>  
>  BuildRequires: python-setuptools
> +BuildRequires: numpy
> +BuildRequires: python-matplotlib
> +BuildRequires: python-mako
>  Requires: nfs-utils 
>  Requires: python-matplotlib
>  Requires: numpy
That makes senses.... 

Spec file and source rpm have on http://steved.fedorapeople.org/nfsometer/ have been updated.
Comment 7 Michael Scherer 2012-07-27 12:17:00 EDT
Same 403 as first comment.
Comment 8 Steve Dickson 2012-07-27 15:48:57 EDT
(In reply to comment #7)
> Same 403 as first comment.
Please try it again... Selinux strikes again!
Comment 9 Michael Scherer 2012-07-29 06:17:47 EDT
There is 2 rpmlint warning :

nfsometer.x86_64: W: non-standard-group System Tools

nfsometer.x86_64: E: no-binary

You need to add Arch: noarch ( since that's a noarch rpm ), and fix the group ( even if i am not sure of the status of the group in Fedora, as there is no policy, 

The license tag is also wrong, should be GPLv2+ ( since there is a "or later" clause )



Running it show that it may use iozone and bonnie++, should they be added as Requires or as a documentation somehwere ? ( also, it may need git, make, tar and other stuff, so I am not sure on how to proceed there ) 



Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: EXTRA Spec file according to URL is the same as in SRPM.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[-]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[-]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[!]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)" For detailed output of licensecheck see file:
     /home/misc/checkout/git/FedoraReview/nfsometer/nfsometer/licensecheck.txt
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[!]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD SourceX tarball generation or download is documented.
[x]: SHOULD SourceX / PatchY prefixed with %{name}.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues
------
[!]: MUST License field in the package spec file matches the actual license.
[!]: MUST Useful -debuginfo package or justification otherwise.

Rpmlint
-------
Checking: nfsometer-debuginfo-1.1-1.fc17.x86_64.rpm
          nfsometer-1.1-1.fc17.src.rpm
          nfsometer-1.1-1.fc17.x86_64.rpm
nfsometer-debuginfo.x86_64: E: empty-debuginfo-package
nfsometer.src: W: non-standard-group System Tools
nfsometer.src:1: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 1)
nfsometer.x86_64: E: explicit-lib-dependency python-matplotlib
nfsometer.x86_64: W: non-standard-group System Tools
nfsometer.x86_64: E: no-binary
3 packages and 0 specfiles checked; 3 errors, 3 warnings.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:
Requires
--------
nfsometer-debuginfo-1.1-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    

nfsometer-1.1-1.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    /bin/bash  
    /usr/bin/python  
    filebench  
    nfs-utils  
    numpy  
    python(abi) = 2.7
    python-mako  
    python-matplotlib  

Provides
--------
nfsometer-debuginfo-1.1-1.fc17.x86_64.rpm:
    
    nfsometer-debuginfo = 1.1-1.fc17
    nfsometer-debuginfo(x86-64) = 1.1-1.fc17

nfsometer-1.1-1.fc17.x86_64.rpm:
    
    nfsometer = 1.1-1.fc17
    nfsometer(x86-64) = 1.1-1.fc17

MD5-sum check
-------------
http://www.linux-nfs.org/~dros/nfsometer/releases/nfsometer-1.1.tar.gz :
  MD5SUM this package     : efbca65ec863b63cb852baac5ff409cf
  MD5SUM upstream package : efbca65ec863b63cb852baac5ff409cf


Generated by fedora-review 0.2.0 (a5c4ced) last change: 2012-07-22
Command line :../try-fedora-review -n nfsometer
External plugins:
Comment 10 Steve Dickson 2012-07-30 08:53:28 EDT
(In reply to comment #9)
> There is 2 rpmlint warning :
> 
> nfsometer.x86_64: W: non-standard-group System Tools
> 
> nfsometer.x86_64: E: no-binary
> 
> You need to add Arch: noarch ( since that's a noarch rpm ), and fix the
> group ( even if i am not sure of the status of the group in Fedora, as there
> is no policy, 
I'm assuming you mean a "BuildArch: noarch"
I changed the group to: "Group: Applications/System"

> 
> The license tag is also wrong, should be GPLv2+ ( since there is a "or
> later" clause )
The License is now: "License: GPLv2+"

> 
> 
> 
> Running it show that it may use iozone and bonnie++, should they be added as
> Requires or as a documentation somehwere ? ( also, it may need git, make,
> tar and other stuff, so I am not sure on how to proceed there ) 
Those will be used if they exist... The README talks about which workloads
exist by default and which ones are optional... So I think this is good 
to go... 

I believe I have all the rpmlint warnings removed:

$ rpmlint nfsometer-1.1-1.fc17.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint nfsometer.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.


Spec file and source rpm have on http://steved.fedorapeople.org/nfsometer/ have been updated (and restorecon-ed ;-) )
Comment 11 Michael Scherer 2012-07-30 09:12:15 EDT
Indeed, the last issue were fixed, so approved.
Comment 12 Steve Dickson 2012-07-30 09:33:53 EDT
(In reply to comment #11)
> Indeed, the last issue were fixed, so approved.

Thank you for your insight and time!
Comment 13 Michael Scherer 2012-08-01 17:30:59 EDT
Steve, didn't you forgot to ask for the package to be added to git
( https://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages ) ?
Comment 14 Steve Dickson 2012-08-02 11:27:33 EDT
New Package SCM Request
=======================
Package Name: NFSometer
Short Description: NFS Performance Framework Tool
Owners: steved@fedorapeople.org
Branches: f18
InitialCC:
Comment 15 Gwyn Ciesla 2012-08-02 11:54:37 EDT
Use FAS account, not email address.  Names in summary and SCM request don't
match, please correct, and don't request f18, it's not branched yet and
master is automatic.
Comment 16 Steve Dickson 2012-08-06 07:43:20 EDT
(In reply to comment #15)
> Use FAS account, not email address.  Names in summary and SCM request don't
> match, please correct, and don't request f18, it's not branched yet and
> master is automatic.
Is this better?

New Package SCM Request
=======================
Package Name: NFSometer
Short Description: NFS Performance Framework Tool
Owners: steved
Branches: 
InitialCC:
Comment 17 Gwyn Ciesla 2012-08-06 09:14:04 EDT
The names still don't match, should it be partially capitalized, or all
lowercase?
Comment 18 Steve Dickson 2012-08-06 10:29:04 EDT
(In reply to comment #17)
> The names still don't match, should it be partially capitalized, or all
> lowercase?
I know case mattered... how is this one?

New Package SCM Request
=======================
Package Name: nfsometer
Short Description: NFS Performance Framework Tool
Owners: steved
Branches: 
InitialCC:
Comment 19 Gwyn Ciesla 2012-08-06 10:32:43 EDT
Git done (by process-git-requests).

Perfect, thanks!
Comment 20 Steve Dickson 2012-08-21 11:24:38 EDT
(In reply to comment #19)
> Git done (by process-git-requests).
> 
> Perfect, thanks!

fedpkg clone nfsometer seems to work but no other fedpkg commands (like fedpkg prep or fedpkg local) seem to work... What do I need to do?
Comment 21 Gwyn Ciesla 2012-08-21 11:32:28 EDT
Have you done fedpkg import of your SRPM?
Comment 22 Steve Dickson 2012-08-21 11:43:16 EDT
(In reply to comment #21)
> Have you done fedpkg import of your SRPM?

I guess I didn't know I need to do that... I just assumed (wrongly) that would happen when the git tree was created. 

So I've don the import on both the master and f18 branch as well as the fedpkg build... is there anything else I need to do to ensure the package is included in f18? tia!!
Comment 23 Gwyn Ciesla 2012-08-21 11:47:58 EDT
Submit an update bodhi for the f18 build.

https://admin.fedoraproject.org/updates

Also, best practice is to fedpkg import the SRPM on master, then fedpkg switch-branch f18, git merge master, fedpkg push, fedpkg build.
Comment 24 David Juran 2013-01-14 07:45:15 EST
Package Change Request
======================
Package Name: nfsometer
New Branches: el6
Owners: steved djuran
Comment 25 Gwyn Ciesla 2013-01-14 08:26:22 EST
Git done (by process-git-requests).

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