Bug 593839 - Review Request: python-urwid - Console user interface library
Review Request: python-urwid - Console user interface library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Peter Lemenkov
Fedora Extras Quality Assurance
: Reopened
: 510097 (view as bug list)
Depends On:
Blocks: 593841
  Show dependency treegraph
 
Reported: 2010-05-19 15:53 EDT by David Cantrell
Modified: 2010-05-25 17:38 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-05-25 17:38:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
lemenkov: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Cantrell 2010-05-19 15:53:13 EDT
Spec URL: http://dcantrel.fedorapeople.org/urwid/urwid.spec
SRPM URL: http://dcantrel.fedorapeople.org/urwid/urwid-0.9.9.1-1.fc13.src.rpm
Description:
Urwid is a Python library for making text console applications.  It has many
features including fluid interface resizing, support for UTF-8 and CJK
encodings, standard and custom text layout modes, simple markup for setting
text attributes, and a powerful, dynamic list box that handles a mix of widget
types.  It is flexible, modular, and leaves the developer in control.

Purpose:
urwid is similar to newt-python in that it is a text-based widget library built on top of curses.  It is required for the wicd-curses package, which is a curses-based frontend for the wicd client.  See the wicd package review request for details on wicd.
Comment 1 Chen Lei 2010-05-20 05:25:30 EDT

*** This bug has been marked as a duplicate of bug 510097 ***
Comment 2 Chen Lei 2010-05-20 05:34:16 EDT
I suggest you to move to #510097, cwickert already take review for this package. Since the reporter is non-responsive for a long time, you can take that review, I think it'll be approved soon.
Comment 3 David Cantrell 2010-05-20 11:21:43 EDT

*** This bug has been marked as a duplicate of bug 510097 ***
Comment 4 David Cantrell 2010-05-20 11:22:37 EDT
*** Bug 510097 has been marked as a duplicate of this bug. ***
Comment 5 Chen Lei 2010-05-20 11:27:31 EDT
Sorry for the inconvience.

I think this package should be renamed to python-urwid

See http://fedoraproject.org/wiki/PackageNamingGuidelines#Addon_Packages_.28python_modules.29
Comment 6 Peter Lemenkov 2010-05-20 13:07:13 EDT
Taking this for review.
Comment 7 Peter Lemenkov 2010-05-20 13:44:59 EDT
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

First, I would like to point out that package cannot be built on F-11/12 (and EPEL 4/5) due to missing %{python_sitearch} definition on these platforms. So you should keep this in mind while requesting cvs branches.

Also, I after the Chen Lei would like to see this package renamed to python-urwid. You may add additional "Provides: urwid = %{version}-%{release}" also.

- rpmlint is NOT silent:

Sulaco ~/rpmbuild/SPECS: rpmlint ../RPMS/ppc/urwid-*
urwid.ppc: W: spelling-error %description -l en_US resizing -> residing, re sizing, re-sizing
urwid.ppc: W: spelling-error %description -l en_US encodings -> encoding, encoding s, encodes
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/container.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/listbox.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/escape.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/split_repr.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/signals.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/util.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/command_map.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/display_common.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/monitored_list.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/__init__.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/old_str_util.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/canvas.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/main_loop.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/curses_display.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/font.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/text_layout.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/html_fragment.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/raw_display.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/graphics.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/web_display.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/wimp.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/widget.py 0644 /usr/bin/python
urwid.ppc: E: non-executable-script /usr/lib/python2.6/site-packages/urwid/decoration.py 0644 /usr/bin/python
2 packages and 0 specfiles checked; 23 errors, 2 warnings.
Sulaco ~/rpmbuild/SPECS:

This line can help suppressing most of these messages

find urwid -type f -name "*.py" -exec sed -i '/^#!\/usr\/bin\/python/d' {} \;

and this one - fixes permissions:
find urwid -type f -name "*.py" -exec chmod 644 {} \;

However I'm not aware of initial intention - it seems that many of these *.py sources may be used as simple scripts while testing (just grep for __main__ and see the results). We still don't use %check rpm target very often, so I'm leaving this up to David to decide what to do with these files and rpmlint errors. 

The former two rpmlint warnings should be omitted, obviously.

- The package must be named according to the Package Naming Guidelines. See Chen Lei's comment #5 .
- The spec file name must match the base package %{name}, in the format %{name}.spec. the same as above.

+ The package meets the Packaging Guidelines .
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines .
+ The License field in the package spec file matches the actual license (LGPLv2+).
0 The package doesn't include the text of the license(s) in its own file.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as provided in the spec URL.

Sulaco ~/rpmbuild/SOURCES: sha256sum urwid-0.9.9.1.tar.gz*
81c95440f84a90872d5bd8f01bc507cd0e5e1ce67a878a62cb435a662e43d5a5  urwid-0.9.9.1.tar.gz
81c95440f84a90872d5bd8f01bc507cd0e5e1ce67a878a62cb435a662e43d5a5  urwid-0.9.9.1.tar.gz.1
Sulaco ~/rpmbuild/SOURCES:

+ The package successfully compiles and builds into binary rpms on at least one primary architecture (my ppc - still primary for F-12).
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
0 No shared library files (not just symlinks) in any of the dynamic linker's default paths.
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.

+/- Permissions on files must be set properly. See my comments about shebang and file permissions above.

+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 The package does not contain library files with a suffix.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives,
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
+ All filenames in the package are valid UTF-8.

Almost finished. Please, rename package (consider adding additional provides also) and comment my remarks about rpmlint warning and file permissions, and I'll continue.
Comment 8 David Cantrell 2010-05-20 21:32:29 EDT
(In reply to comment #7)
> REVIEW:
> 
> Legend: + = PASSED, - = FAILED, 0 = Not Applicable
> 
> First, I would like to point out that package cannot be built on F-11/12 (and
> EPEL 4/5) due to missing %{python_sitearch} definition on these platforms. So
> you should keep this in mind while requesting cvs branches.

Added those macros in.

> Also, I after the Chen Lei would like to see this package renamed to
> python-urwid. You may add additional "Provides: urwid = %{version}-%{release}"
> also.

Renamed the package to python-urwid.  I did not add the Provides for urwid since this is the first time the package is being added.

> - rpmlint is NOT silent:
> 
> Sulaco ~/rpmbuild/SPECS: rpmlint ../RPMS/ppc/urwid-*
> urwid.ppc: W: spelling-error %description -l en_US resizing -> residing, re
> sizing, re-sizing
> urwid.ppc: W: spelling-error %description -l en_US encodings -> encoding,
> encoding s, encodes
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/container.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/listbox.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/escape.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/split_repr.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/signals.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/util.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/command_map.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/display_common.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/monitored_list.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/__init__.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/old_str_util.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/canvas.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/main_loop.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/curses_display.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/font.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/text_layout.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/html_fragment.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/raw_display.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/graphics.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/web_display.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/wimp.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/widget.py 0644 /usr/bin/python
> urwid.ppc: E: non-executable-script
> /usr/lib/python2.6/site-packages/urwid/decoration.py 0644 /usr/bin/python
> 2 packages and 0 specfiles checked; 23 errors, 2 warnings.
> Sulaco ~/rpmbuild/SPECS:
> 
> This line can help suppressing most of these messages
> 
> find urwid -type f -name "*.py" -exec sed -i '/^#!\/usr\/bin\/python/d' {} \;
> 
> and this one - fixes permissions:
> find urwid -type f -name "*.py" -exec chmod 644 {} \;

Added these two find lines to %prep.

> However I'm not aware of initial intention - it seems that many of these *.py
> sources may be used as simple scripts while testing (just grep for __main__ and
> see the results). We still don't use %check rpm target very often, so I'm
> leaving this up to David to decide what to do with these files and rpmlint
> errors. 

There is a test_urwid.py script at the top level which performs unit tests on the module.  I added a %check section that runs this script.  I left the other _test functions and classes in the module code.

New spec file and SRPM uploaded:
http://dcantrel.fedorapeople.org/python-urwid/python-urwid.spec
http://dcantrel.fedorapeople.org/python-urwid/python-urwid-0.9.9.1-1.fc13.src.rpm
Comment 9 Chen Lei 2010-05-20 23:07:16 EDT
1. I think %doc CHANGELOG *.html is not enough, I suggest you remove useless file before %file and add examples.

%install
...

%{__rm} -f  tmpl_tutorial.html
%{__mkdir} examples
%{__cp} -p *.py examples
%{__rm} examples/test_urwid.py


%files

%doc CHANGELOG *.html examples

2. 

Packaing tricks has a better command that can remove both /usr/bin/python and /usr/bin/env python shebang from *.py files.

See 
https://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_files
Comment 10 Peter Lemenkov 2010-05-21 01:07:40 EDT
(In reply to comment #9)

> Packaing tricks has a better command that can remove both /usr/bin/python and
> /usr/bin/env python shebang from *.py files.
> 
> See 
> https://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_files    

Well, they are almost the same, so I don't think they are better - I'd rather say that they are slightly different :)


(In reply to comment #8)

> There is a test_urwid.py script at the top level which performs unit tests on
> the module.  I added a %check section that runs this script.  I left the other
> _test functions and classes in the module code.
> 
> New spec file and SRPM uploaded:
> http://dcantrel.fedorapeople.org/python-urwid/python-urwid.spec
> http://dcantrel.fedorapeople.org/python-urwid/python-urwid-0.9.9.1-1.fc13.src.rpm    

There is one new issue - lack of "BuildRequires: python-twisted-core". I also suspect, that there is also lack of "Requires: python-twisted-core" - take a look at the output of 

$ grep twisted urwid/*py
Comment 11 Chen Lei 2010-05-21 01:30:00 EDT
(In reply to comment #10)
> (In reply to comment #9)
> There is one new issue - lack of "BuildRequires: python-twisted-core". I also
> suspect, that there is also lack of "Requires: python-twisted-core" - take a
> look at the output of 
> $ grep twisted urwid/*py    

Agree, python-urwid should require pygobject and python-twisted-core since rpmbuild cannot pick up python module dependencies automatically.  I think "BuildRequires: python-twisted-core" is not needed, because mock is happy without this BR.
Comment 12 Chen Lei 2010-05-21 01:49:18 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > There is one new issue - lack of "BuildRequires: python-twisted-core". I also
> > suspect, that there is also lack of "Requires: python-twisted-core" - take a
> > look at the output of 
> > $ grep twisted urwid/*py    
> Agree, python-urwid should require pygobject and python-twisted-core since
> rpmbuild cannot pick up python module dependencies automatically.  I think
> "BuildRequires: python-twisted-core" is not needed, because mock is happy
> without this BR.    

Sorry, I revoke my comment #c11. From http://excess.org/urwid, I found those third-party python modules for urwid are all optional.

System Requirements¶
•Python 2.3 - 2.6 
•Linux, OSX, Cygwin or other unix-like operating system 
•(optional) ncurses library to use the curses_display module 
•(optional) Apache 1.3 or higher to use the web_display module 
•(optional) python-glib to use the GlibEventLoop class 
•(optional) Twisted to use the TwistedEventLoop class 

Also, docgen_*.py are not examples for this package and should be removed in %doc.
Comment 13 Christoph Wickert 2010-05-21 05:52:36 EDT
(In reply to comment #9)
> %{__rm} -f  tmpl_tutorial.html
> %{__mkdir} examples
> %{__cp} -p *.py examples
> %{__rm} examples/test_urwid.py

Please don't use macros for trivial things like rm, mkdir or cp. This was discussed on fedora-devel recently and people agreed these macros offer no benefit, see http://lists.fedoraproject.org/pipermail/devel/2010-March/133466.html
Comment 14 Chen Lei 2010-05-21 06:03:05 EDT
(In reply to comment #13)
> (In reply to comment #9)
> > %{__rm} -f  tmpl_tutorial.html
> > %{__mkdir} examples
> > %{__cp} -p *.py examples
> > %{__rm} examples/test_urwid.py
> 
> Please don't use macros for trivial things like rm, mkdir or cp. This was
> discussed on fedora-devel recently and people agreed these macros offer no
> benefit, see
> http://lists.fedoraproject.org/pipermail/devel/2010-March/133466.html    

I agree with you to not use those macros, but someone may not be agree with this.

See https://bugzilla.redhat.com/show_bug.cgi?id=592670

I use those macros here just to keep packaging style close to David :)
Comment 15 Mamoru TASAKA 2010-05-21 06:24:54 EDT
.... and who banned using such macros?
Comment 16 David Cantrell 2010-05-21 18:54:30 EDT
I have updated the spec file and SRPM with the changes noted in comments 9 through 15.  Here's a summary:

1) Discontinue the use of basic command macros.
2) Creation and inclusion of the examples/ in the %doc list.
3) Change to using the slightly different sed command to remove the #! line.

One concern raised is whether or not python-urwid should require python-twisted-core or pygobject2.  I cannot tell what the conclusion on that was, so I did not add either.  My personal take on it that since the module wraps the import statements in try/except blocks, python-urwid should be fine.  If users want the twisted functionality, they can install the python-twisted-core package.  If anything dependent on python-urwid requires the twisted usage inside python-urwid, then that package can Require both python-urwid and python-twisted-core.

New spec and SRPM:
http://dcantrel.fedorapeople.org/python-urwid/python-urwid.spec
http://dcantrel.fedorapeople.org/python-urwid/python-urwid-0.9.9.1-1.fc13.src.rpm

Thanks for all the review feedback!
Comment 17 Chen Lei 2010-05-21 23:55:42 EDT

If you want to pass %check, you must BR:python-twisted-core pygobject2 which will pull in a lot of dependencies. I think you may need to either remove %check from spec or add _with_check to spec to disable %check by default.
Comment 18 Peter Lemenkov 2010-05-22 00:40:09 EDT
(In reply to comment #16)

> One concern raised is whether or not python-urwid should require
> python-twisted-core or pygobject2.  I cannot tell what the conclusion on that
> was, so I did not add either.  My personal take on it that since the module
> wraps the import statements in try/except blocks, python-urwid should be fine. 
> If users want the twisted functionality, they can install the
> python-twisted-core package.  If anything dependent on python-urwid requires
> the twisted usage inside python-urwid, then that package can Require both
> python-urwid and python-twisted-core.

Then you *must* edit ./test_urwid.py and exclude checks dependent on twisted. You can also add twisted as buildrequires and not as requires (just for faking test results), but I don't think it's a good idea.

Otherwise package looks good.
Comment 19 David Cantrell 2010-05-22 02:59:05 EDT
I added BuildRequires for python-twisted-core and pygobject2.  New spec and SRPM:

http://dcantrel.fedorapeople.org/python-urwid/python-urwid.spec
http://dcantrel.fedorapeople.org/python-urwid/python-urwid-0.9.9.1-1.fc13.src.rpm

Thanks!
Comment 20 Peter Lemenkov 2010-05-22 03:33:20 EDT
Ok, good. This package is

APPROVED.
Comment 21 David Cantrell 2010-05-22 04:13:17 EDT
New Package CVS Request
=======================
Package Name: python-urwid
Short Description: Console user interface library
Owners: dcantrel
Branches: F-13 EL-6
InitialCC:
Comment 22 Dennis Gilmore 2010-05-25 16:58:25 EDT
CVS Done

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