[Bug 472229] Review Request: PyQwt - Python bindings for Qwt

[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=472229


Tadej Janež <tadej.janez at tadej.hicsalta.si> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?(tadej.janez at tadej |
                   |.hicsalta.si)               |




--- Comment #4 from Tadej Janež <tadej.janez at tadej.hicsalta.si>  2009-01-09 15:51:39 EDT ---
Firstly, thank you for taking the time to review my package and I apologize for
not responding to your review earlier.

(In reply to comment #2)
> Some notes:
> 
> * BuildRequires/Requires 
>   - build.log shows
> ------------------------------------------------------------
>     80  Failed to import numarray: PyQwt will be build without support for
> numarray.
>     81  Failed to find Numeric2: PyQwt will be build without support for
> Numeric.
> .....
>     86   'disable_numarray': False,
>     87   'disable_numeric': False,
>     88   'disable_numpy': False,
>  ------------------------------------------------------------
>     These message seems contradictory.
>     Perhaps "BuildRequires: python-numarray python-numeric"
>     is needed.

I would prefer to avoid all this and just disable support for Numarray and
Numeric at configure stage. As far as I know, Numpy is deemed as a replacement
for both Numarray and Numeric.

>   - Also please check if numpy (or python-numarray, python-numeric)
>     are also needed for "Requires" (not BuildRequires). 
>     For python module related packages, writing a package in 
>     "BuildRequires" does not mean that the package is installed
>     at runtime.

Yes, it is definitely needed at run time, so I added it to "Requires".

>   - And check if PyQwt-devel does not require "qwt-devel".
>     For example, QwtList.sip contains:
> -------------------------------------------------------------
>     34  %MappedType QList<QwtPickerMachine::Command>
>     35  {
>     36  %TypeHeaderCode
>     37  #include <qlist.h>
>     38  #include <qwt_picker_machine.h>
>     39  %End
> -------------------------------------------------------------

Yes, you are right. I added qwt-devel to PyQwt-devel's "Requires".

> * Timestamps
>   - 'INSTALL="install -p"' argument to "make install" does not
>     make sense for this package because this makefile is not
>     based on autotools but based on python
>     (actually when installing files "cp -f" is used)

I don't completely understand how (not) using autotools affects the use of
"install -p" option, but I trust you that it is not necessary.

> 
> * Permission of scripts
> -------------------------------------------------------------
> # non-executable script
> chmod 755 %{buildroot}%{python_sitearch}/PyQt4/Qwt5/grace.py
> -------------------------------------------------------------
>   - If this script is not meant to be called directly by
>     users (but is meant to be called internally from programs
>     or so), then this script should not have executable
>     permission and the shebang should be removed.

grace.py could be executed directly but it requires 'grace' package to work.
If the user doesn't have it installed, it just crashes with an error that
'xmgrace' is not found. However, it seems bloated to me to add a new
'Requires: grace' just because some example script needs it.

Also, upstream is a bit confusing about executable scripts, because a similar
example script qplt.py could also have a shebang at the top and be made
executable but it doesn't have it.
I made grace.py executable because I received warning messages from rpmlint
and I decided that would be the simplest solution.
Maybe it is better to make all example scripts consistent and non-executable?
If so, I should remove the shebang from grace.py.

Updated spec file and source package are at
http://tadej.hicsalta.si/packages/

-- 
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.