Red Hat Bugzilla – Bug 927611
Review Request: pyode - Open-source Python bindings for The Open Dynamics Engine
Last modified: 2013-05-22 17:40:31 EDT
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]$
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
(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
> > - 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...
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
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!
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
Hi John, Thanks for the patch. I'll update the srpm/spec tonight and post updated versions. Thanks, Warm regards, Ankur
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
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.
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)]
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
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
Updated ChangeLog too Ankur
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:
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:
Git done (by process-git-requests).
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
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
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
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
pyode-1.2.0-4.fc19 has been pushed to the Fedora 19 testing repository.
pyode-1.2.0-4.el6 has been pushed to the Fedora EPEL 6 testing repository.
pyode-1.2.0-4.el6 has been pushed to the Fedora EPEL 6 stable repository.