Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: python-py - Innovative python library containing py.test, greenlets and other niceties
https://bugzilla.redhat.com/show_bug.cgi?id=459800
Summary: Review Request: python-py - Innovative python library containing py.test, greenlets and other niceties Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: thomas.moschny@gmx.de QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
Spec URL: http://thm.fedorapeople.org/python-py.spec SRPM URL: http://thm.fedorapeople.org/python-py-0.9.1-1.fc9.src.rpm
Description: The py lib aims at supporting a decent development process addressing deployment, versioning, testing and documentation perspectives.
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=459800
--- Comment #1 from Jason Tibbitts tibbs@math.uh.edu 2008-09-04 21:42:32 EDT --- I have to admit, it's terribly funny to "%define srcname py" and then go on to use "%{srcname}" in place of "py". Ten keystrokes (twelve if you count the shift key) to replace two? But that's your option as the maintainer.
0.9.2 came out the same day you opened this ticket; did you want to update the package?
With a license like that, you really need to be more specific about which parts have which license. Otherwise the poor reviewer has to repeat the entire process you used to figure out the licensing information along with anyone who may maintain it in the future.
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=459800
--- Comment #2 from Thomas Moschny thomas.moschny@gmx.de 2008-09-07 10:40:09 EDT --- (In reply to comment #1)
I have to admit, it's terribly funny to "%define srcname py" and then go on to use "%{srcname}" in place of "py". Ten keystrokes (twelve if you count the shift key) to replace two? But that's your option as the maintainer.
Well, there are more reasons for defining macros than solely reducing the number of keystrokes. Normally I use %{name} e.g. for the Url and Source tags, but in case of python packages %{name} doesn't work because of the leading 'python-', that's why there is %{srcname}.
Anyway, I removed %{srcname} here.
0.9.2 came out the same day you opened this ticket; did you want to update the package?
Thanks for the hint. Updating the package indeed simplified packaging a bit, because upstream made some efforts towards a clean install using setuptools.
With a license like that, you really need to be more specific about which parts have which license. Otherwise the poor reviewer has to repeat the entire process you used to figure out the licensing information along with anyone who may maintain it in the future.
The specfile now has a more detailed list wrt licensing.
Spec URL: http://thm.fedorapeople.org/python-py.spec SRPM URL: http://thm.fedorapeople.org/python-py-0.9.2-1.fc9.src.rpm
%changelog * Sun Sep 7 2008 Thomas Moschny thomas.moschny@gmx.de - 0.9.2-1 - Update to 0.9.2. - Upstream now uses setuptools and installs to %%{python_sitearch}. - Remove %%{srcname} macro. - More detailed information about licenses.
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=459800
Toshio Ernie Kuratomi a.badger@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |a.badger@gmail.com AssignedTo|nobody@fedoraproject.org |a.badger@gmail.com Flag| |fedora-review?
--- Comment #3 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-09-29 20:18:29 EDT --- NEEDSWORK
Good: * Named according to the package naming guidelines and spec matches * Licensed appropriately: Regarding your note about test/web/post_multipart.py, you probably want to add a link to this: http://code.activestate.com/help/terms/. (That would be Python for this code snippet) * Spec file is legible. * Tarball matches upstream. * Not a shared library package * Builds in mock on i386 * package owns all directories it creates and no others * all filenames are utf-8 * proper %clean * macros used consistently * No duplicate files * package contains code not content
To be fixed: * rpmlint: python-py.i386: E: non-standard-executable-perm /usr/lib/python2.5/site-packages/py/c-extension/greenlet/greenlet.so 0775 3 packages and 0 specfiles checked; 1 errors, 0 warnings.
Not sure why setuptools is making this 0755 instead of 0755 but you can correct it in your spec file: %install [...] chmod 0755 %{buildroot}%{python_sitearch}/py/c-extension/greenlet/greenlet.so
* The package has a compat directory with modules that are also in the stdlibrary. It looks like they're just copies svn around the 2.4.4 release. These have to go.
I'd suggest the following:
- For the Fedora package, our goal is to be able to rm -rf py/compat. We should do this in the spec file. - For upstream we need a patch that first tries system libraries and then fallsback to the py/compat modules if necessary.
When patching the source we probably need to grep for occurrences of "compat" in the source. Then change things like this::
import py
usage = "usage: %prog [-s [filename ...] | [-i | -c filename ...]]" optparser = py.compat.optparse.OptionParser(usage)
to::
import py try: import optparse except ImportError: from py.compat import optparse
usage = "usage: %prog [-s [filename ...] | [-i | -c filename ...]]" optparser = py.compat.optparse.OptionParser(usage)
This second problem is important enough to require fixing before approval.
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=459800
--- Comment #4 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-10-07 20:44:48 EDT --- Oops, that should be:: optparser = optparse.OptionParser(usage)
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=459800
--- Comment #5 from Toshio Kuratomi tkuratom@redhat.com 2008-11-19 21:09:00 EDT --- ping
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=459800
--- Comment #6 from Thomas Moschny thomas.moschny@gmx.de 2008-11-21 13:53:36 EDT --- The py.magic.greenlet code does not work on ppc/ppc64, and the corresponding test segfaults. So far, upstream has not been able to reproduce that, and thus there's no fix yet, that's why we are currently stalled here.
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=459800
--- Comment #7 from Thomas Moschny thomas.moschny@gmx.de 2008-11-21 17:53:07 EDT --- Spec URL: http://thm.fedorapeople.org/python-py.spec SRPM URL: http://thm.fedorapeople.org/python-py-0.9.2-4.fc9.src.rpm
%changelog * Fri Nov 21 2008 Thomas Moschny <..> - 0.9.2-4 - Use dummy_greenlet on ppc and ppc64.
* Tue Oct 7 2008 Thomas Moschny <..> - 0.9.2-3 - Replace compat modules by stubs using the system modules instead. - Add patch from trunk fixing a timing issue in the tests.
* Tue Sep 30 2008 Thomas Moschny <..> - 0.9.2-2 - Update license information. - Fix the tests.
Still four failing tests, though, see http://koji.fedoraproject.org/koji/taskinfo?taskID=944709 http://koji.fedoraproject.org/koji/taskinfo?taskID=944685
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=459800
--- Comment #8 from Thomas Moschny thomas.moschny@gmx.de 2008-11-26 16:11:16 EDT --- Problems reported upstream, see https://codespeak.net/issue/py-dev/issue66 and https://codespeak.net/issue/py-dev/issue67
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=459800
--- Comment #9 from Thomas Moschny thomas.moschny@gmx.de 2008-12-13 17:24:00 EDT --- Spec URL: http://thm.fedorapeople.org/python-py.spec SRPM URL: http://thm.fedorapeople.org/python-py-0.9.2-5.fc10.src.rpm
%changelog * Fri Dec 12 2008 Thomas Moschny <..> - 0.9.2-5 - Add patch from trunk fixing a subversion 1.5 problem (pylib issue66). - Don't replace doctest compat module (pylib issue67).
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=459800
--- Comment #10 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-12-19 22:58:21 EDT --- The doctest failure in apigen.txt seems to be a genuine failure in py that the compat-doctest module is somehow mishandling. If you run the commands in apigen.txt in a python interactive shell, you get the traceback that doctest from python-2.5 yields. I've been looking at this more but haven't gotten to the bottom of it yet.
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=459800
--- Comment #11 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-12-19 23:09:34 EDT --- The example fails in a python-2.5 and python-2.4.3 shell.
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=459800
--- Comment #12 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-12-19 23:18:51 EDT --- Here's a session you can feed back to the upstream bug report. If the opportunity arises, you can also mention that this is a reason not to include copies of system libraries in something they ship ;-)
Python 2.4.3 (#1, Jan 14 2008, 18:32:40) [GCC 4.1.2 20070626 (Red Hat 4.1.2-14)] on linux2 Type "help", "copyright", "credits" or "license" for more information.
import py p = py.path.local('.') p.check(dir=True)
True
from py.__.apigen.tracer.docstorage import DocStorage, DocStorageAccessor from py.__.apigen.tracer.tracer import Tracer toregister = {'py.path.local': py.path.local, 'py.path.svnwc': py.path.svnwc} ds = DocStorage().from_dict(toregister) t = Tracer(ds) t.start_tracing() p = py.path.local('.') p.check(dir=True)
Traceback (most recent call last): File "<stdin>", line 1, in ? File "/home/fedora/toshio/py-0.9.2/py/path/common.py", line 95, in check def check(self, **kw): File "/home/fedora/toshio/py-0.9.2/py/apigen/tracer/tracer.py", line 38, in _tracer self.docstorage.consider_call(frame, None, self.frame) File "/home/fedora/toshio/py-0.9.2/py/apigen/tracer/docstorage.py", line 80, in consider_call desc.consider_call_site(caller_frame, upward_cut_frame) File "/home/fedora/toshio/py-0.9.2/py/apigen/tracer/description.py", line 170, in consider_call_site cs = cut_stack(stack, frame, cut_frame) File "/home/fedora/toshio/py-0.9.2/py/apigen/tracer/description.py", line 81, in cut_stack lst = [py.code.Frame(i) for i in stack[stack.index(frame):\ ValueError: list.index(x): x not in list
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=459800
--- Comment #13 from Toshio Ernie Kuratomi a.badger@gmail.com 2008-12-19 23:40:09 EDT --- Good: * rpmlint is now silent * optparse, textwrap, and subprocess have been switched to system libraries.
Still Needswork: * doctest is still using the compat module due to: https://codespeak.net/issue/py-dev/issue67
Note that this is either a problem in the tracer module or the way we're being told to use the tracer module in apigen.txt, not in the py.test or doctest module as the current upstream bug seems to think.
Where does that leave us?
I think removing the doctest module in favor of system libs is the first step. Then it would be very nice to run the tests with just the apigen.txt test disabled until upstream fixes the bug. (This could be done by creative moving of the apigen.txt doc file before and after the test run although that's not pretty. If there's a command line option to py.test that excludes the one failing test, that would be better.)
Once that's done, I can approve this package.
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=459800
--- Comment #14 from Thomas Moschny thomas.moschny@gmx.de 2009-01-14 15:46:08 EDT --- Thanks for looking into this. I've updated the upstream bug accordingly.
Spec URL: http://thm.fedorapeople.org/python-py.spec SRPM URL: http://thm.fedorapeople.org/python-py-0.9.2-6.fc10.src.rpm
%changelog * Wed Jan 14 2009 Thomas Moschny <..> - 0.9.2-6 - Use system doctest module again, as this wasn't the real cause of the test failure. Instead, remove the failing test for now.
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=459800
Toshio Ernie Kuratomi a.badger@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flag|fedora-review? |fedora-review+
--- Comment #15 from Toshio Ernie Kuratomi a.badger@gmail.com 2009-01-15 12:23:16 EDT --- I'd still ship the apigen documentation. Maybe something like this::
# see pylib issue67 cp -p py/doc/apigen.txt apigen.txt.bak cp -p py/doc/index.txt index.txt.bak rm py/doc/apigen.txt sed -i '/apigen/d' py/doc/index.txt
PYTHONPATH=$(pwd)/py %{__python} py/bin/py.test py cp -p apigen.txt py/doc/apigen.txt.bak cp -p index.txt.bak py/doc/index.txt
You can do this after import, however. This is APPROVED.
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=459800
Thomas Moschny thomas.moschny@gmx.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #16 from Thomas Moschny thomas.moschny@gmx.de 2009-01-15 15:33:29 EDT --- (In reply to comment #15)
I'd still ship the apigen documentation. Maybe something like this::
The testsuite is run in BUILD, not BUILDROOT, (and after %install) so deleting some files there doesn't have an effect on the final package, and thus I don't think it is necessary to make backups.
You can do this after import, however. This is APPROVED.
Thanks for the review, Toshio!
New Package CVS Request ======================= Package Name: python-py Short Description: Innovative python library containing py.test, greenlets and other niceties Owners: thm Branches: F-9 F-10 InitialCC: none Cvsextras Commits: yes
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=459800
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #17 from Kevin Fenzi kevin@tummy.com 2009-01-15 15:41:19 EDT --- cvs done.
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=459800
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |ON_QA
--- Comment #18 from Fedora Update System updates@fedoraproject.org 2009-01-26 20:47:43 EDT --- python-py-0.9.2-6.fc10 has been pushed to the Fedora 10 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update python-py'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-0968
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=459800
--- Comment #19 from Fedora Update System updates@fedoraproject.org 2009-02-04 21:21:23 EDT --- python-py-0.9.2-6.fc10 has been pushed to the Fedora 10 stable repository. If problems still persist, please make note of it in this bug report.
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=459800
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution| |NEXTRELEASE
package-review@lists.fedoraproject.org