Bug 609295 - Review Request: python-cement - CLI Application Framework for Python
Summary: Review Request: python-cement - CLI Application Framework for Python
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 661179 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-29 21:29 UTC by BJ Dierkes
Modified: 2011-01-20 19:55 UTC (History)
2 users (show)

Fixed In Version: python-cement-0.8.14-5.fc13
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-01-20 19:53:00 UTC
Type: ---
Embargoed:
j: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description BJ Dierkes 2010-06-29 21:29:27 UTC
Spec URL: http://5dollarwhitebox.org/tmp/cement.spec
SRPM URL: http://5dollarwhitebox.org/tmp/cement-0.8.6-1.fc12.src.rpm

Description:

Cement is an advanced CLI Application Framework for Python. It promotes code
re-use by way of plugins and helper libraries that can be shared between
any application built on Cement.  The MVC and overall framework design is
very much inspired by the TurboGears2 web framework.  Its goal is to introduce
a standard, and feature-full platform for both simple and complex command
line applications as well as support rapid development needs without sacrificing
quality.

At a minimum, Cement configures the following features for every application:

     * Multiple Configuration file parsing (default: /etc, ~/)
     * Command line argument and option parsing
     * Dual Console/File Logging Support
     * Full Internal and External (3rd Party) Plugin support
     * Basic "hook" support
     * Full MVC support for advanced application design
     * Text output rendering with Genshi templates
     * Json output rendering allows other programs to access your CLI-API

Comment 1 BJ Dierkes 2010-12-07 02:09:06 UTC
Updated:

SPEC: http://5dollarwhitebox.org/tmp/cement.spec
SRPM: http://5dollarwhitebox.org/tmp/cement-0.8.12-1.fc14.src.rpm


$ rpmlint -i SPECS/cement.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -i RPMS/noarch/cement*0.8.12*.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

Comment 2 BJ Dierkes 2010-12-10 01:44:13 UTC
Updated, upstream now includes -devtools and -test under the primary source.  The spec now performs nose testing as well.  Also, renamed as python-cement as it is more appropriate.

SPEC: http://5dollarwhitebox.org/tmp/python-cement.spec
SRPM: http://5dollarwhitebox.org/tmp/python-cement-0.8.14-1.fc14.src.rpm


$ rpmlint -i SPECS/python-cement.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -i RPMS/noarch/python-cement-*
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementapp/LICENSE
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementplugin/LICENSE
3 packages and 0 specfiles checked; 2 errors, 0 warnings.


Note the warning are because the -devtools package includes application templates... the LICENSE and README in those templates are left blank by design.

Comment 3 BJ Dierkes 2010-12-10 01:45:16 UTC
*** Bug 661179 has been marked as a duplicate of this bug. ***

Comment 4 Jason Tibbitts 2010-12-14 16:58:16 UTC
A few comments:

Do I recall correctly that you are the upstream developer of this software?  It would probably have benefited you to let us know about that.

Are you sure you still need to pass --single-version-externally-managed to setuptools?  http://fedoraproject.org/wiki/Packaging:Python_Eggs indicates that this should no longer be required, and that document is a few years old now.

Nothing seems to own %{python_sitelib}/%{real_name}.  You include a few subdirectories but not the directory itself.  And, what does %{real_name} actually buy you, anyway?  Isn't it shorter to simply type "cement"?

It looks like you have a bunch of nasty macroification at the top which is unnecessary for Fedora (or even EL6).  You could dispense with %pyver just by using globs in your file lists, and I'm not sure what the %python macro is used for at all.  Some of the spec is so buried in pointless macros that it becomes quite difficult to understand.

Comment 5 BJ Dierkes 2010-12-15 00:02:18 UTC
Thank you for your feedback.  I can agree with the use of 'real_name' making things more cluttered.  I've cleaned that up.  I've also removed --single-version-externally-managed.

As for the use of %{python} this was done to make porting to other python stacks easier.  Rather than changing all references of 'python' to 'python3' or what-have-you, I can simply change the python macro.  

As for %{pyver}, using wild cards in the files list for 'egg-info' seems it would be less efficient... again when considering that this package may very well be ported to another python stack (via side-by-side python installs, etc).... where pyver might be 2.6 or 3.1 depending on the stack.   I don't see any reason to change that at this point.  That said the updated files are at:

SPEC: http://5dollarwhitebox.org/tmp/python-cement.spec
SRPM: http://5dollarwhitebox.org/tmp/python-cement-0.8.14-2.fc14.src.rpm

$ rpmlint -i SPECS/python-cement.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -i RPMS/noarch/python-cement-*
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementapp/LICENSE
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementplugin/LICENSE
3 packages and 0 specfiles checked; 2 errors, 0 warnings.

Comment 6 BJ Dierkes 2010-12-15 00:05:36 UTC
Oh by the way, yes I am also the upstream author.  I didn't think to mention that initially, but will with any future submissions.  Thanks.

Comment 7 Jason Tibbitts 2010-12-15 01:14:01 UTC
Unfortunately the %{python} macro doesn't save you all the trouble, because you still need your build dependencies to include python2-devel on Fedora (instead of python-devel).

Also, if you intend to do the python3 thing, did you consider using the method in the current guidelines that builds packages for both python 2 and 3 from a single package?  It's even more loaded with macros but gets you the whole thing at once.

Comment 8 BJ Dierkes 2010-12-15 01:44:33 UTC
You're right about the python2-devel bit.  I've removed the %{python} macro now... thanks for your patience.

As for building for both python2 and python3, yes it was a thought.  However upstream the project is not ready for python3 yet....  the next version of cement upstream will be targeted at python3 and therefore will probably have cement-0.X on python2, and the next version on python3.


SPEC: http://5dollarwhitebox.org/tmp/python-cement.spec
SRPM: http://5dollarwhitebox.org/tmp/python-cement-0.8.14-3.fc14.src.rpm

$ rpmlint -i SPECS/python-cement.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -i RPMS/noarch/python-cement-*
python-cement-devtools.noarch: E: zero-length
/usr/lib/python2.7/site-packages/cement/paste/templates/cementapp/LICENSE
python-cement-devtools.noarch: E: zero-length
/usr/lib/python2.7/site-packages/cement/paste/templates/cementplugin/LICENSE
3 packages and 0 specfiles checked; 2 errors, 0 warnings.


Thanks again.

Comment 9 Jason Tibbitts 2010-12-16 20:32:37 UTC
Everything does look much cleaner, thanks.  (Although you don't have to make changes just because I suggest that they might look cleaner; it's a dialogue and I'm not making any demands.)

I still think that pyver is unnecessary and can be replaced by globs, but it isn't a really big deal.

Can you explain what pkguytil.py is for?  I'm trying to understand why it needs to be there (since all non-EOL Fedora releases have python >= 2.6, as does EL6), whether it gets used and whether it could be removed.  The question is whether it runs afoul of our bundled library policy.

Note that the License: tag stuff should be from the perspective of the built rpms, so when listing the license bits you should give the location of the file as it's installed, instead of where it appears in the source tree.


* source files match upstream.  sha256sum:
  4053518c5fe884f9b7d8b363ee06f3273b2274aff7be66cedcc9e25a26aa83ba
   cement-0.8.14.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* rpmlint has acceptable complaints.
* final provides and requires are sane:
  python-cement-0.8.14-3.fc15.noarch.rpm
   python-cement = 0.8.14-3.fc15
  =
   python  
   python(abi) = 2.7
   python-configobj  
   python-genshi  
   python-jsonpickle  

  python-cement-devtools-0.8.14-3.fc15.noarch.rpm
   python-cement-devtools = 0.8.14-3.fc15
  =
   python(abi) = 2.7
   python-cement = 0.8.14-3.fc15
   python-paste-script  
   python-tempita  

  python-cement-doc-0.8.14-3.fc15.noarch.rpm
   python-cement-doc = 0.8.14-3.fc15
  =
   (none)

* %check is present and all tests pass:
   Ran 72 tests in 0.122s
   OK

? pkgutil.py might be bundled.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files

Comment 10 BJ Dierkes 2010-12-17 00:49:41 UTC
Thanks, and I appreciate the discussion and recommendations.  I don't take it as a demand for changes or anything.

I've updated the License comment.  Regarding pkgutil.py ... basically it is required to achieve python 2.4 compatibility in RHEL 5 (this is destined for EPEL 5 as well as Fedora).  The pkgutil library was introduced after Python 2.4 and is required by Cement to handle Genshi text file templating.  I would be more than happy to remove it in %prep for Python >= 2.6 ... but that wouldn't change the license or any functionality (as it isn't used when Python >= 2.6).

SPEC: http://5dollarwhitebox.org/tmp/python-cement.spec
SRPM: http://5dollarwhitebox.org/tmp/python-cement-0.8.14-4.fc14.src.rpm

Comment 11 Jason Tibbitts 2010-12-22 18:02:38 UTC
Sorry for the delay; it's a busy time of year.

The primary issue with pkgutil.py is its interaction with our guidelines against bundling code that should be packaged separately (or is already packaged separately).  Those guidelines are at http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries and https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries

https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries#Conditionalized_functionality seems to me to cover the current situation explicitly.  Can you verity that the bundled pkgutil.py is not used at all for sufficiently new python, and add the comment required by that guideline?  With that, I think we're done.

Comment 12 BJ Dierkes 2011-01-04 01:04:39 UTC
I also apologize for the delay do to the holidays.  I've updated the spec with a comment regarding the bundled 'pkgutil':

SPEC: http://5dollarwhitebox.org/tmp/python-cement.spec
SRPM: http://5dollarwhitebox.org/tmp/python-cement-0.8.14-5.fc14.src.rpm


$ rpmlint -i SPECS/python-cement.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint -i /var/lib/mock/fedora-14-x86_64/result/python-cement*0.8.14-5*.rpm
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementapp/LICENSE
python-cement-devtools.noarch: E: zero-length /usr/lib/python2.7/site-packages/cement/paste/templates/cementplugin/LICENSE
4 packages and 0 specfiles checked; 2 errors, 0 warnings..

Comment 13 Jason Tibbitts 2011-01-05 16:13:39 UTC
Thanks, looks good.

APPROVED

Comment 14 BJ Dierkes 2011-01-05 20:26:45 UTC
New Package SCM Request
=======================
Package Name: python-cement
Short Description: CLI Application Framework for Python
Owners: derks
Branches: f13 f14 el5 el6
InitialCC:

Comment 15 Jason Tibbitts 2011-01-06 14:39:57 UTC
Git done (by process-git-requests).

Comment 16 Fedora Update System 2011-01-11 02:23:39 UTC
python-cement-0.8.14-5.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/python-cement-0.8.14-5.fc14

Comment 17 Fedora Update System 2011-01-11 02:23:59 UTC
python-cement-0.8.14-5.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/python-cement-0.8.14-5.fc13

Comment 18 Fedora Update System 2011-01-12 05:20:35 UTC
python-cement-0.8.14-5.fc13 has been pushed to the Fedora 13 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-cement'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/python-cement-0.8.14-5.fc13

Comment 19 Fedora Update System 2011-01-20 19:52:54 UTC
python-cement-0.8.14-5.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 20 Fedora Update System 2011-01-20 19:55:31 UTC
python-cement-0.8.14-5.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.


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