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=691114
Martin Gieseking martin.gieseking@uos.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.gieseking@uos.de
--- Comment #3 from Martin Gieseking martin.gieseking@uos.de 2011-04-13 15:12:26 EDT --- David, good job. Your informal review looks fine. When reviewing Python packages, please also check the additional items listed here: http://fedoraproject.org/wiki/Packaging:Python#How_to_package
A few additional comments:
(In reply to comment #1)
It'd be good if the nosetests provided were run in %check. I had a quick attempt at making them run but didn't get it working.
Right. Running the tests would be nice if they are up-to-date with the module sources. You can execute them with PYTHONPATH="%{buildroot}%{python_sitearch}" nosetests -w test in a %check section. Unfortunately, they currently crash with a segfault. Fabian, please ask upstream whether this is a bug in the Python module or a test-only issue.
The permissions of the .so file should be set to 755 (see rpmlint output):
$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm python-msgpack.src: W: spelling-error Summary(en_US) serializer -> serialize, serializes, serialized python-msgpack.x86_64: W: spelling-error Summary(en_US) serializer -> serialize, serializes, serialized python-msgpack.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/msgpack/_msgpack.so _msgpack.so()(64bit) python-msgpack.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/msgpack/_msgpack.so 0775L 3 packages and 0 specfiles checked; 1 errors, 3 warnings.
The README states that Cython is required to build msgpack, but the try/except ImportError in setup.py suggests otherwise. I successfully built the package both with and without Cython...
The tarball contains the cython-created .c file. So there's no need recreate it.