Bug 927611 (pyode) - Review Request: pyode - Open-source Python bindings for The Open Dynamics Engine
Summary: Review Request: pyode - Open-source Python bindings for The Open Dynamics Engine
Keywords:
Status: CLOSED ERRATA
Alias: pyode
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: John Morris
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: pybrain
TreeView+ depends on / blocked
 
Reported: 2013-03-26 10:31 UTC by Ankur Sinha (FranciscoD)
Modified: 2013-05-22 21:40 UTC (History)
3 users (show)

Fixed In Version: pyode-1.2.0-4.el6
Clone Of:
Environment:
Last Closed: 2013-05-22 03:19:05 UTC
Type: ---
Embargoed:
john: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
fedora-review results with reviewer notes (7.93 KB, text/plain)
2013-04-15 06:06 UTC, John Morris
no flags Details
Fix GC-related segfault (1.12 KB, patch)
2013-04-24 05:30 UTC, John Morris
no flags Details | Diff
patch to fix rounding errors in %check script (4.62 KB, patch)
2013-04-25 19:37 UTC, John Morris
no flags Details | Diff

Description Ankur Sinha (FranciscoD) 2013-03-26 10:31:04 UTC
Spec URL: http://ankursinha.fedorapeople.org/pyode/pyode.spec
SRPM URL: http://ankursinha.fedorapeople.org/pyode/pyode-1.2.0-1.fc20.src.rpm

Description: 
A set of open-source Python bindings for The Open Dynamics Engine, an
open-source physics engine. PyODE also includes an XODE parser


Fedora Account System Username: ankursinha

[ankur@dhcppc1  SRPMS]$ rpmlint ../SPECS/pyode.spec ./pyode-1.2.0-1.fc18.src.rpm /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm                     ../SPECS/pyode.spec: W: invalid-url Source0: http://downloads.sourceforge.net/pyode/PyODE-snapshot-2010-03-22.tar.gz timed out
pyode.src: W: invalid-url URL: http://pyode.sourceforge.net/ <urlopen error timed out>
pyode.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 3 warnings.
[ankur@dhcppc1  SRPMS]$

Comment 1 John Morris 2013-04-14 20:59:54 UTC
Hi Ankur, I was just about to package this, but you've done it for me!  I'll be happy to help with the review.  What are you doing with this?  I want it for PyCAM (also in review).

Quick drive-by before the real review:

- This URL may help with the 'W: invalid-url' problem
https://downloads.sourceforge.net/project/pyode/pyode/snapshot-2010-03-22/PyODE-snapshot-2010-03-22.tar.gz

- If you remove execute permissions from ode.so, you won't have to bother with the '%{?filter_setup:...' business.  (Why do you set them in %install?)

- Nitpick:  %description lacking a trailing '.'.

- Nitpick:  (I was dinged for this on my last pkg) Double spaces before %description, %prep, %build, etc. make the specfile more readable.

- The %files section really does need blank lines to separate it from the %install script.

- Those comments about the tests might be more clear if they were prefaced with a line like this that indicates they're the reason for not running tests, despite the existence of a 'tests' directory:
#%%check

Comment 2 Ankur Sinha (FranciscoD) 2013-04-15 03:55:05 UTC
(In reply to comment #1)
> Hi Ankur, I was just about to package this, but you've done it for me!  I'll
> be happy to help with the review.  What are you doing with this?  I want it
> for PyCAM (also in review).

The pybrain package recommends using pyode :)

> 
> Quick drive-by before the real review:
> 
> - This URL may help with the 'W: invalid-url' problem
> https://downloads.sourceforge.net/project/pyode/pyode/snapshot-2010-03-22/
> PyODE-snapshot-2010-03-22.tar.gz

Thanks. Updated.

> 
> - If you remove execute permissions from ode.so, you won't have to bother
> with the '%{?filter_setup:...' business.  (Why do you set them in %install?)

To get rid of this rpmlint error:
pyode.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/ode.so 0775L

The permissions must be correct, and since it's an internally used shared object, it must be removed from provides.

> 
> - Nitpick:  %description lacking a trailing '.'.

Corrected

> 
> - Nitpick:  (I was dinged for this on my last pkg) Double spaces before
> %description, %prep, %build, etc. make the specfile more readable.

I don't think this is required. The spec is readable enough :)

> 
> - The %files section really does need blank lines to separate it from the
> %install script.

Added.

> 
> - Those comments about the tests might be more clear if they were prefaced
> with a line like this that indicates they're the reason for not running
> tests, despite the existence of a 'tests' directory:
> #%%check
Added phony check section line.


[ankur@localhost  SRPMS]$ rpmlint ../SPECS/pyode.spec ./pyode-1.2.0-2.fc18.src.rpm /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm
pyode.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 1 warnings.
[ankur@localhost  SRPMS]$


New spec/srpm:
http://ankursinha.fedorapeople.org/pyode/pyode.spec

http://ankursinha.fedorapeople.org/pyode/pyode-1.2.0-2.fc20.src.rpm

Thanks,
Warm regards,
Ankur

Comment 3 John Morris 2013-04-15 06:00:31 UTC
> > - If you remove execute permissions from ode.so, you won't have to bother
> > with the '%{?filter_setup:...' business.  (Why do you set them in %install?)
> 
> To get rid of this rpmlint error:
> pyode.x86_64: E: non-standard-executable-perm
> /usr/lib64/python2.7/site-packages/ode.so 0775L
> 
> The permissions must be correct, and since it's an internally used shared
> object, it must be removed from provides.

Hmm, I learned something new.  For regular packages containing modules, if the module perms are set 0644, the filter_provides_in macro ignores them.  Not so with python modules.

> > - Nitpick:  (I was dinged for this on my last pkg) Double spaces before
> > %description, %prep, %build, etc. make the specfile more readable.
> 
> I don't think this is required. The spec is readable enough :)

Sure.

> > - Those comments about the tests might be more clear if they were prefaced
> > with a line like this that indicates they're the reason for not running
> > tests, despite the existence of a 'tests' directory:
> > #%%check
> Added phony check section line.

I'm addressing %check differently in the review.

Stay tuned...

Comment 4 John Morris 2013-04-15 06:06:53 UTC
Created attachment 735771 [details]
fedora-review results with reviewer notes

Here's the review.  These comments may also be found at the bottom of the attached file.

Reviewer notes
----------------------------------------

[!]: 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.

Rpmlint:  pyode.x86_64: W: no-documentation

- No documentation

%doc should include AUTHORS ChangeLog LICENSE LICENSE-BSD README examples


----------
[!]: Requires correct, justified where necessary.

- Need Requires:  ode and Pyrex


----------
[!]: Package meets the Packaging Guidelines::Python

- Python3 support: Infra is in specfile; does it work, and if so,
  should it be turned on?


----------
[!]: %check is present and all tests pass.

- Has this package been tested?  This %check script fails with a
  segfault, as does running the tests after install:

%check
export PYTHONPATH=build/lib.linux-%{_target_cpu}-%{python_version}
%{__python} tests/test_xode.py


----------
Rpmlint:  pyode.x86_64: W: non-standard-group Unspecified

- Main package missing Group: tag (though python3 package includes
  it).  I want this to be included in EPEL, where Group: is not
  optional.

http://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Comment 5 John Morris 2013-04-23 15:58:36 UTC
Hi Ankur, let me know if there's anything I can do to help.  The point about %check above is not as straight-forward.  I'll check with upstream about it.

Thanks!

Comment 6 John Morris 2013-04-24 05:30:34 UTC
Created attachment 739265 [details]
Fix GC-related segfault

This patch fixes the xode test segfault described toward the end of comment #4.

More information:
http://comments.gmane.org/gmane.comp.python.pyode.user/174

The check can be enabled as such with all tests passing, and no final segfault:

%check
export PYTHONPATH=build/lib.linux-%{_target_cpu}-%{python_version}
%{__python} tests/test_xode.py

I am notifying upstream, and sending the patch in unified format as an attachment.  :P

Comment 7 Ankur Sinha (FranciscoD) 2013-04-25 01:25:34 UTC
Hi John,

Thanks for the patch. I'll update the srpm/spec tonight and post updated versions.

Thanks,
Warm regards,
Ankur

Comment 8 Ankur Sinha (FranciscoD) 2013-04-25 12:28:58 UTC
I've applied the patch, but the tests still fail:

+ /usr/bin/python tests/test_xode.py
..................F............................FF........................F.F..................................
======================================================================
FAIL: testPlaneParams (__main__.TestGeomParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 674, in testPlaneParams
    self.assertEqual(self.geom4.getParams(), ((0.0, 1.0, 0.0), 17.0))
AssertionError: Tuples differ: ((0.0, 0.9999999403953552, 0.0... != ((0.0, 1.0, 0.0), 17.0)
First differing element 0:
(0.0, 0.9999999403953552, 0.0)
(0.0, 1.0, 0.0)
- ((0.0, 0.9999999403953552, 0.0), 16.999998092651367)
+ ((0.0, 1.0, 0.0), 17.0)
======================================================================
FAIL: testAMotorAxes1 (__main__.TestJointParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 588, in testAMotorAxes1
    self.assertEqual(ref, axis1)
AssertionError: Tuples differ: (0.0, 1.0, 0.0) != (0.0, 0.9999999403953552, 0.0)
First differing element 1:
1.0
0.999999940395
- (0.0, 1.0, 0.0)
+ (0.0, 0.9999999403953552, 0.0)
======================================================================
FAIL: testAMotorAxes3 (__main__.TestJointParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 594, in testAMotorAxes3
    self.assertEqual(ref, axes)
AssertionError: Lists differ: [(1.0, 0.0, 0.0), (0.0, 1.0, 0... != [(0.9999999403953552, 0.0, 0.0...
First differing element 0:
(1.0, 0.0, 0.0)
(0.9999999403953552, 0.0, 0.0)
- [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)]
+ [(0.9999999403953552, 0.0, 0.0),
+  (0.0, 0.9999999403953552, 0.0),
+  (0.0, 0.0, 0.9999999403953552)]
======================================================================
FAIL: testHingeAxis (__main__.TestJointParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 534, in testHingeAxis
    self.assertEqual(self.joint5.getAxis(), (1.0, 0.0, 0.0))
AssertionError: Tuples differ: (0.9999999403953552, 0.0, 0.0) != (1.0, 0.0, 0.0)
First differing element 0:
0.999999940395
1.0
- (0.9999999403953552, 0.0, 0.0)
+ (1.0, 0.0, 0.0)
======================================================================
FAIL: testSliderAxis (__main__.TestJointParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 540, in testSliderAxis
    self.assertEqual(self.joint6.getAxis(), (0.0, 1.0, 0.0))
AssertionError: Tuples differ: (0.0, 0.9999999403953552, 0.0) != (0.0, 1.0, 0.0)
First differing element 1:
0.999999940395
1.0
- (0.0, 0.9999999403953552, 0.0)
+ (0.0, 1.0, 0.0)
----------------------------------------------------------------------
Ran 110 tests in 0.502s
FAILED (failures=5)
RPM build errors:


Not sure what to do about this. I guess we'll have to have another chat with upstream.

Anyway, updated spec/srpm here:
http://ankursinha.fedorapeople.org/pyode/pyode.spec

http://ankursinha.fedorapeople.org/pyode/pyode-1.2.0-3.fc19.src.rpm

At least the packaging part looks OK now.

Thanks,
Warm regards,
Ankur

Comment 9 John Morris 2013-04-25 19:29:41 UTC
Heh, differs by 6e-8, so it fails.  Patch coming up.

I realize that Pyrex should not be in the Requires: section, only in the BuildRequires: section.  My apologies, I was wrong about that.

Comment 10 John Morris 2013-04-25 19:37:28 UTC
Created attachment 740048 [details]
patch to fix rounding errors in %check script

Fix rounding-error test failures on Fedora 17-20 (but not el6!) (64-bit only?)

Differences of 5*10^-8, as shown in below output, cause tests to fail.

Fix by adding new check 'assertFloatListsAlmostEqual' which
recursively sorts through lists/tuples and compares floats with
'assertAlmostEqual'.

http://docs.python.org/2/library/unittest.html#unittest.TestCase.assertAlmostEqual


======================================================================
FAIL: testAMotorAxes3 (__main__.TestJointParser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_xode.py", line 594, in testAMotorAxes3
    self.assertEqual(ref, axes)
AssertionError: Lists differ: [(1.0, 0.0, 0.0), (0.0, 1.0, 0... != [(0.9999999403953552, 0.0, 0.0...
First differing element 0:
(1.0, 0.0, 0.0)
(0.9999999403953552, 0.0, 0.0)
- [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)]
+ [(0.9999999403953552, 0.0, 0.0),
+  (0.0, 0.9999999403953552, 0.0),
+  (0.0, 0.0, 0.9999999403953552)]

Comment 11 John Morris 2013-04-25 19:42:18 UTC
These two should be the last items.  Thanks for hanging on during this whole process!

I'd like to be co-maintainer, by the way, and I'll be happy to help run this through the process of getting into the stable repos.

    John

Comment 12 Ankur Sinha (FranciscoD) 2013-04-26 03:23:35 UTC
Hi John,

Updated spec/srpm:

http://ankursinha.fedorapeople.org/pyode/pyode-1.2.0-4.fc20.src.rpm

http://ankursinha.fedorapeople.org/pyode/pyode.spec

I'm more than happy to co-maintain the package with you. I don't generally maintain packages for epel any way :)

* Fri Apr 26 2013 Ankur Sinha <ankursinha AT fedoraproject DOT org> 1.2.0-4
- Add another patch to use almost equal assertion
- Fix wrong end of line file encoding rpmlint error


I also removed pyrex from Requires:, but forgot to add it to the changelog :|

Thanks,
Warm regards, 
Ankur

Comment 13 Ankur Sinha (FranciscoD) 2013-04-26 03:27:16 UTC
Updated ChangeLog too

Ankur

Comment 14 John Morris 2013-04-26 22:30:07 UTC
Looks great, Ankur.  Thanks for the hard work!

APPROVED

Here's the SCM request to move things along.

New Package SCM Request
=======================
Package Name: pyode
Short Description: Open-source Python bindings for The Open Dynamics Engine
Owners: ankursinha zultron
Branches: f17 f18 f19 f20 el6
InitialCC:

Comment 15 Ankur Sinha (FranciscoD) 2013-04-27 00:19:14 UTC
Thanks John. Setting the scm flag and pasting the request text (just to ensure that the reporter does it, not sure the script checks but better to be safe :) )

New Package SCM Request
=======================
Package Name: pyode
Short Description: Open-source Python bindings for The Open Dynamics Engine
Owners: ankursinha zultron
Branches: f17 f18 f19 f20 el6
InitialCC:

Comment 16 Gwyn Ciesla 2013-04-27 20:46:39 UTC
Git done (by process-git-requests).

Comment 17 Fedora Update System 2013-04-29 02:08:33 UTC
pyode-1.2.0-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/pyode-1.2.0-4.fc17

Comment 18 Fedora Update System 2013-04-29 02:08:47 UTC
pyode-1.2.0-4.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/pyode-1.2.0-4.fc19

Comment 19 Fedora Update System 2013-04-29 02:08:59 UTC
pyode-1.2.0-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/pyode-1.2.0-4.el6

Comment 20 Fedora Update System 2013-04-29 02:09:09 UTC
pyode-1.2.0-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/pyode-1.2.0-4.fc18

Comment 21 Fedora Update System 2013-04-29 17:43:46 UTC
pyode-1.2.0-4.fc19 has been pushed to the Fedora 19 testing repository.

Comment 22 Fedora Update System 2013-05-22 03:19:05 UTC
pyode-1.2.0-4.el6 has been pushed to the Fedora EPEL 6 testing repository.

Comment 23 Fedora Update System 2013-05-22 21:40:31 UTC
pyode-1.2.0-4.el6 has been pushed to the Fedora EPEL 6 stable repository.


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