Informal review:

OK source files match upstream:
5095f72025132711c472b5a0db6417be13492b6ce09a74dfea9afe27165a6000  rootplot-1.1.tar.gz

OK package meets naming and versioning guidelines.
NOT OK specfile is properly named, is cleanly written and uses macros consistently.
OK dist tag is present.
OK build root is correct.
OK license field matches the actual license.
OK license is open source-compatible.
license text not included upstream.
NOT OK latest version is being packaged.
OK BuildRequires are proper.
OK %clean is present.
OK package builds in koji.
http://koji.fedoraproject.org/koji/taskinfo?taskID=2608031
OKpackage installs properly.
OK rpmlint is silent.
(3 spellcheck warnings which can be ignored.)
OK final provides and requires are sane:
rpm -q --provides rootplot
rootplot = 1.1-1.fc14
pm -q --requires rootplot
/usr/bin/python  
numpy  
python(abi) = 2.7
python-matplotlib  
root-python  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
N/A %check is present and all tests pass:
N/A no shared libraries are added to the regular linker search paths.
OK owns the directories it creates.
NOT OK doesn't own any directories it shouldn't.
OK no duplicates in %files.
OK file permissions are appropriate.
OK no scriptlets present.
OK documentation is small, so no -docs subpackage is necessary.
OK %docs are not necessary for the proper functioning of the package.
OK no headers.
OK no pkgconfig files.
OK no libtool .la droppings.
N/A desktop files valid and installed properly.

So the remaining issues are:

- Use macros rather than vars consistently (e.g. buildroot).
- You can use "python" directly.
- The latest upstream version is 2.2.
- Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of sitelib.

Also, running the first example

http://packages.python.org/rootplot/plot_directive/pyplots/first.py

through python gives

Traceback (most recent call last):
  File "first.py", line 1, in <module>
    import rootplot.root2matplotlib as r2m
ImportError: No module named rootplot.root2matplotlib

Adjusting the import line results in a segmentation violation, which may very well be ROOT's fault.

As a more constructive side remark:

Rebuilding the rpms with current source (rootplot 2.2) and adjusting as necessary makes the mentioned example run without a glitch. So this will be OK with an updated spec.

Actually rpmlint output is
rootplot.noarch: E: explicit-lib-dependency python-matplotlib
rootplot.noarch: W: spelling-error Summary(en_US) matplotlib -> matelote, matrilocal, marplot
rootplot.noarch: W: spelling-error %description -l en_US pyROOT -> pyrostat, pyrope, pyrone
rootplot.noarch: W: spelling-error %description -l en_US matplotlib -> matelote, matrilocal, marplot
rootplot.noarch: W: no-manual-page-for-binary rootplot
rootplot.noarch: W: no-manual-page-for-binary tree2hists
rootplot.src: W: spelling-error Summary(en_US) matplotlib -> matelote, matrilocal, marplot
rootplot.src: W: spelling-error %description -l en_US pyROOT -> pyrostat, pyrope, pyrone
rootplot.src: W: spelling-error %description -l en_US matplotlib -> matelote, matrilocal, marplot
2 packages and 0 specfiles checked; 1 errors, 8 warnings.

All of these are OK, though. The explicit lib dependency is only relevant in binary packages, where RPM automatically adds dependencies on the necessary shared libraries.

(In reply to comment #1)
> So the remaining issues are:
> 
> - Use macros rather than vars consistently (e.g. buildroot).

Actually, the spec file does not mix styles. The only thing this applies is using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The choice between these is a matter of personal preference.

> - You can use "python" directly.

Yes, there's not much sense in using macros for standard stuff, such as %{__rm},  %{__mv}, %{__mkdir}, %{__mkdir_p} and so on even if they are implemented in RPM. Although, using them is not forbidden.

> - The latest upstream version is 2.2.
> - Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of
> sitelib.

This is not a problem per se, the rootplot package only ends up owning the directories that are in %{python_sitelib} *in the build root*.

What ends up being owned by
 %{python_sitelib}/*
is (on F14)
 /usr/lib/python2.7/site-packages/root2matplotlib
 /usr/lib/python2.7/site-packages/rootplot-1.1-py2.7.egg-info
 /usr/lib/python2.7/site-packages/rootplot_scripts

Still, IMHO using wildcards when they are not absolutely necessary is a bit of bad style; for instance you won't notice if e.g. egg-info isn't built for some reason. I highly recommend being more verbose, i.e. using
 %{python_sitelib}/root2matplotlib/
 %{python_sitelib}/rootplot_scripts/
 %{python_sitelib}/rootplot-%{version}-py*.egg-info
instead of the needlessly general wildcard glob.


- Patch0 is missing a comment in the spec file. Please document its purpose, e.g.

# Patch to remove shbangs from Python library
Patch0:         rootplot-rm-shbangs.patch


- I'm not sure why you want to use the conditionals in

%if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%endif

I'd scrap the %if stuff altogether.

***

Review:

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK

MUST: The License field in the package spec file must match the actual license. OK
- License is MIT Modern style with sublicense
http://fedoraproject.org/wiki/Licensing/MIT

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
016f11836fb3764dd89dd5f0c290816b  rootplot-1.1.tar.gz
016f11836fb3764dd89dd5f0c290816b  ../SOURCES/rootplot-1.1.tar.gz

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A
MUST: Optflags are used and time stamps preserved. OK
MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. ~OK
- Maybe add PKG-INFO to %doc?

MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
SHOULD: %{?dist} tag is used in release. OK

SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. NEEDSWORK
- No license texts in the tarball.

SHOULD: The package builds in mock. OK
EPEL: Clean section exists. OK
EPEL: Buildroot cleaned before install. OK
EPEL: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A


**

Please tend to the issues reported above before import to git. Especially update to newest upstream as instructed by Michael in comment #2, and check that running the examples work.

This package has been

APPROVED

(In reply to comment #3)
> Actually rpmlint output is
> rootplot.noarch: E: explicit-lib-dependency python-matplotlib
> rootplot.noarch: W: spelling-error Summary(en_US) matplotlib -> matelote,
> matrilocal, marplot
> rootplot.noarch: W: spelling-error %description -l en_US pyROOT -> pyrostat,
> pyrope, pyrone
> rootplot.noarch: W: spelling-error %description -l en_US matplotlib ->
> matelote, matrilocal, marplot
> rootplot.noarch: W: no-manual-page-for-binary rootplot
> rootplot.noarch: W: no-manual-page-for-binary tree2hists
> rootplot.src: W: spelling-error Summary(en_US) matplotlib -> matelote,
> matrilocal, marplot
> rootplot.src: W: spelling-error %description -l en_US pyROOT -> pyrostat,
> pyrope, pyrone
> rootplot.src: W: spelling-error %description -l en_US matplotlib -> matelote,
> matrilocal, marplot
> 2 packages and 0 specfiles checked; 1 errors, 8 warnings.

On my side that was rpmlint on spec and srpm. Sorry.

> > - Use macros rather than vars consistently (e.g. buildroot).
> 
> Actually, the spec file does not mix styles. The only thing this applies is
> using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The
> choice between these is a matter of personal preference.

It uses different styles for different variables ($RPM_BUILD_ROOT vs. %{_bindir}). If that's not a problem I'm fine with it.

> > - Use %{python_sitelib}/root2matplotlib etc. so that you do not own all of
> > sitelib.
> 
> This is not a problem per se, the rootplot package only ends up owning the
> directories that are in %{python_sitelib} *in the build root*.

I've heard someone say

- The %files section needs improvement. The statement
 %{python_sitearch}/
ends up owning the system python directory, which is not wanted.

Is this different?

(In reply to comment #4)
> (In reply to comment #3)
> > Actually, the spec file does not mix styles. The only thing this applies is
> > using $RPM_OPT_FLAGS vs %{optflags} and $RPM_BUILD_ROOT vs %{buildroot}. The
> > choice between these is a matter of personal preference.
> 
> It uses different styles for different variables ($RPM_BUILD_ROOT vs.
> %{_bindir}). If that's not a problem I'm fine with it.

Yes, this is no problem. There's no shell variable declared by rpmbuild to point to %{_bindir}. However, the functions of $RPM_BUILD_ROOT and %{buildroot} are identical, and so are those of $RPM_OPT_FLAGS and %{optflags}.

> I've heard someone say
> 
> - The %files section needs improvement. The statement
>  %{python_sitearch}/
> ends up owning the system python directory, which is not wanted.
> 
> Is this different?

You had
 %{python_sitearch}/
which indeed ends up owning the system python directory. Here Steve has
 %{python_sitearch}/*
which only ends up owning whatever is in the system python directory of the buildroot.

(In reply to comment #5)
> (In reply to comment #4)
> > I've heard someone say
> > 
> > - The %files section needs improvement. The statement
> >  %{python_sitearch}/
> > ends up owning the system python directory, which is not wanted.
> > 
> > Is this different?
> 
> You had
>  %{python_sitearch}/
> which indeed ends up owning the system python directory. Here Steve has
>  %{python_sitearch}/*
> which only ends up owning whatever is in the system python directory of the
> buildroot.

The star makes the difference. Thanks for the clarification!



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Michael and Jussi, all very valuable, thankyou.


I've tried to comment on everything:

__ONE__
>NOT OK specfile is properly named, is cleanly written and uses macros
>consistently.

This was use of $RPM_BUILD_ROOT consistently but also use of %{_bindir}
that was questioned.
As was commented this is fine unlike using $RPM_BUILD_ROOT and %{buildroot}.

I have change %{} everywhere just as it is tidier I agree.

__TWO__
>NOT OK latest version is being packaged.
I have updated to  2.2.1

__THREE__
> You can use "python" directly.
I could for sure,... but the reason for choosing %{__python} is that
I hope to offer the package on EPEL5 with python26 pending root appearing
for python26 within EPEL5 and then this will make it easier to build
a secondary module for python26.

__FOUR__
Also, running the first example
http://packages.python.org/rootplot/plot_directive/pyplots/first.py
through python gives
Traceback (most recent call last):
  File "first.py", line 1, in <module>
    import rootplot.root2matplotlib as r2m
ImportError: No module named rootplot.root2matplotlib

As commented this has been corrected with latest upstream.

__FIVE__
>Still, IMHO using wildcards when they are not absolutely necessary is a bit of
>bad style; for instance you won't notice if e.g. egg-info isn't built for some
>reason. I highly recommend being more verbose, i.e. using
>%{python_sitelib}/root2matplotlib/
>%{python_sitelib}/rootplot_scripts/
>%{python_sitelib}/rootplot-%{version}-py*.egg-info
>instead of the needlessly general wildcard glob.

Okay I agree using 
%{python_sitelib}/*
 is lazy.

I have gone for a middle ground of

%{python_sitelib}/rootplot-%{version}-*
%dir %{python_sitelib}/rootplot
%{python_sitelib}/rootplot/*

and all these name spaces are definitely mine as it were.

__SIX__
>Patch0 is missing a comment in the spec file. Please document 
 its purpose.

Done.
# Removes some #!/usr/bin/env python from some python libs.
Patch0:         rootplot-rm-shbangs.patch

__SEVEN__
>>I'm not sure why you want to use the conditionals in

>>%if ! (0%{?fedora} > 12 || 0%{?rhel} > 5)
>>%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from
>> distutils.sysconfig import get_python_lib; print(get_python_lib())")}
>>%endif

I add conditionals since it's then obvious that  in the future when
RHEL5 and FEDORA12 are dead  and berried this can be removed completely.
... I'm keeping it.


http://cern.ch/straylen/rpms/rootplot/rootplot.spec
http://cern.ch/straylen/rpms/rootplot/rootplot-2.2.1-1.fc13.src.rpm


are new packages, I'll leave them here for a week or so before requesting
GIT in case you have any comments.

Thanks again, Steve.

(In reply to comment #7)
> __FIVE__
> >Still, IMHO using wildcards when they are not absolutely necessary is a bit of
> >bad style; for instance you won't notice if e.g. egg-info isn't built for some
> >reason. I highly recommend being more verbose, i.e. using
> >%{python_sitelib}/root2matplotlib/
> >%{python_sitelib}/rootplot_scripts/
> >%{python_sitelib}/rootplot-%{version}-py*.egg-info
> >instead of the needlessly general wildcard glob.
> 
> Okay I agree using 
> %{python_sitelib}/*
>  is lazy.
> 
> I have gone for a middle ground of
> 
> %{python_sitelib}/rootplot-%{version}-*
> %dir %{python_sitelib}/rootplot
> %{python_sitelib}/rootplot/*
> 
> and all these name spaces are definitely mine as it were.

.. does this even build? You're missing the directory
 %{python_sitelib}/rootplot_scripts/
along with its contents.

As a stylistic issue, I don't see any point in using
 %dir %{python_sitelib}/rootplot
 %{python_sitelib}/rootplot/*
since both
 %{python_sitelib}/rootplot
or
 %{python_sitelib}/rootplot/
have the same function. I prefer the latter version, which demonstrates that this is a directory.

> I add conditionals since it's then obvious that  in the future when
> RHEL5 and FEDORA12 are dead  and berried this can be removed completely.
> ... I'm keeping it.

OK. I'm just saying nothing breaks even if you don't use the conditionals at all and just declare the macro as usual.

RHEL5 is going to be around for a long time, still. F12 is practically dead already, it's EOLin in a couple of weeks.

(In reply to comment #8)
> .. does this even build? You're missing the directory
>  %{python_sitelib}/rootplot_scripts/
> along with its contents.
>

rootplot 2.2 has a different (simpler, better) directory structure, there's only rootplot/ and the egg.

Interesting I had always assumed that 
> %{python_sitelib}/rootplot
>or
> %{python_sitelib}/rootplot/
were different but you are correct the second one does include the directory
as well.

Have opted for 

%{python_sitelib}/rootplot/

for the same reasons as you suggest.

http://cern.ch/straylen/rpms/rootplot/rootplot.spec
http://cern.ch/straylen/rpms/rootplot/rootplot-2.2.1-2.fc13.src.rpm

Will request in a couple of days.

Thanks again.

Steve.

New Package SCM Request
=======================
Package Name: rootplot
Short Description: Plots ROOT data with matplotlib
Owners: stevetraylen
Branches: f13 f14 el6
InitialCC:


I'll make a el5 if when root gets to el5.

Git done (by process-git-requests).

rootplot-2.2.1-3.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc14

rootplot-2.2.1-3.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc13

rootplot-2.2.1-3.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rootplot'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.fc13

rootplot-2.2.1-3.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

rootplot-2.2.1-3.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: rootplot
New Branches: el5
Owners: stevetraylen

p.s I noticed that el5 is branched from devel where as el6 is branched
from f12 which does not obviously make sense.

Maybe both should come from devel now.

Steve.

Git done (by process-git-requests).

rootplot-2.2.1-3.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.el5

rootplot-2.2.1-3.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/rootplot-2.2.1-3.el6

rootplot-2.2.1-3.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

rootplot-2.2.1-3.el6 has been pushed to the Fedora EPEL 6 stable repository.  If problems still persist, please make note of it in this bug report.