[Bug 608332] Review Request: rootplot - Plots ROOT data with matplotlib

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=608332

--- Comment #7 from Steve Traylen <steve.traylen at cern.ch> 2010-11-19 16:34:57 EST ---
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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.