Bug 1186389 - Review Request: python-mako1.0 - Mako template library for Python
Summary: Review Request: python-mako1.0 - Mako template library for Python
Keywords:
Status: ON_QA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Toshio Ernie Kuratomi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1179743
TreeView+ depends on / blocked
 
Reported: 2015-01-27 15:24 UTC by Matěj Cepl
Modified: 2018-04-11 06:29 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:
a.badger: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Matěj Cepl 2015-01-27 15:24:34 UTC
Spec URL: http://mcepl.fedorapeople.org/tmp/python-mako10.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/7935/8737935/python-mako10-1.0.0-1.el6.src.rpm
Description: 
Mako is a template library written in Python. It provides a familiar,
non-XML syntax which compiles into Python modules for maximum
performance. Mako's syntax and API borrows from the best ideas of many
others, including Django templates, Cheetah, Myghty, and Genshi.
Conceptually, Mako is an embedded Python (i.e. Python Server Page)
language, which refines the familiar ideas of componentized layout and
inheritance to produce one of the most straightforward and flexible
models available, while also maintaining close ties to Python calling
and scoping semantics.

Fedora Account System Username: mcepl

Comment 2 Matěj Cepl 2015-01-28 11:51:26 UTC
Note, that this package should be EPEL only.

Comment 3 Toshio Ernie Kuratomi 2015-01-28 15:58:40 UTC
Things I noticed right away (Will do a formal review too):

* Name would be better as python-mako1.0  Using the dotless version is the old way of doing things but since it's unclear (is that for mako version 1.0 or mako version 10?) using dots is preferred for new packages.
* Download location doesn't work: I think mako must have switched to using pypi dor tarball hosting at some point and the main mako package hasn't adjusted (it should make that change as well)
* Latest version is 1.0.1: https://pypi.python.org/packages/source/M/Mako/Mako-1.0.1.tar.gz
* The filter is not needed because there won't be any .so's in a noarch python package.
* The mako-render program will need to be renamed so that it doesn't conflict with the non forward-compat mako-render.  The rules about how to construct that name changed recently so I'll have to look it up when I do the full review.

Comment 4 Toshio Ernie Kuratomi 2015-01-28 17:41:24 UTC
* rpmlint:
python-mako10.src: W: spelling-error Summary(en_US) Mako -> Mao, Mayo, Macho
python-mako10.src: W: spelling-error %description -l en_US Mako -> Mao, Mayo, Macho
python-mako10.src: W: spelling-error %description -l en_US Mako's -> Mao's, Mayo's, Macho's
python-mako10.src: W: spelling-error %description -l en_US componentized -> component
python-mako10.src: W: invalid-url Source0: http://www.makotemplates.org/downloads/Mako-1.0.0.tar.gz HTTP Error 404: Not Found
python-mako10.noarch: W: spelling-error Summary(en_US) Mako -> Mao, Mayo, Macho
python-mako10.noarch: W: spelling-error %description -l en_US Mako -> Mao, Mayo, Macho
python-mako10.noarch: W: spelling-error %description -l en_US Mako's -> Mao's, Mayo's, Macho's
python-mako10.noarch: W: spelling-error %description -l en_US componentized -> component
python-mako10.noarch: W: no-manual-page-for-binary mako-render
python-mako10-doc.noarch: W: spelling-error %description -l en_US Mako -> Mao, Mayo, Macho
python-mako10-doc.noarch: W: spelling-error %description -l en_US Mako's -> Mao's, Mayo's, Macho's
python-mako10-doc.noarch: W: spelling-error %description -l en_US componentized -> component
python-mako10.spec: W: invalid-url Source0: http://www.makotemplates.org/downloads/Mako-1.0.0.tar.gz HTTP Error 404: Not Found
3 packages and 2 specfiles checked; 0 errors, 14 warnings.

* Spelling errors are all fine as they're all legal to use in this contect.
* man page is a warning but up to you if you want to add one.  Personally I'd see if the main python-mako packager wants to add one.  I think that Debian/Ubuntu has one so they could possibly copy that (or generate it if that's all that Debian is doing).
* Invalid source is a problem.  mentioned in previous comment that they seem to have moved to: https://pypi.python.org/packages/source/M/Mako/Mako-%{version}.tar.gz

Good:
* spec filename matches package name
* LICENSE file is included
* Spec file is legible
* Source matches upstream but see invalid source_url note
* Package builds in koji
* No localized files provided so no handling needed
* Not an elf library so no special handling needed
* No bundled libraries
* Package is not relocatable
* Package owns all directories it creates
* Files are not listed twice
* Package does not own files or dirs of other packages
* Permissions set appropriately
* macros used consistently
* Package contains code
* API Docs split off into separate package
* No %doc documentation is used at runtime
* Not a GUI app so no special handling
* All filenames are valid utf-8

Needs fixing:
* The invalid source_url from rpmlint
* License tag for the javascript seems outdated, no longer under the GPL.  I do see BSD license references though.  So probably needs to be updated to: 
    # The documentation contains javascript for search which are licensed BSD and MIT
    License: (MIT and Python) and (BSD and GPLv2)
* Need to change from unversioned python macros to versioned macros.  Like this:

At the top:

%if 0%{?rhel} && 0%{?rhel} <= 6
%{!?__python2: %global __python2 /usr/bin/python2}
%{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%endif

In the spec file:
 * s/__python/__python2/g
 * s/python_sitelib/python2_sitelib/g
 

 * mako-render needs to be renamed to be parallel installable.  Since we don't currently need to worry about python3 here, either of these of these would be fine:
   - mako-render-1.0
   - mako-render-v1.0
 
Judgement calls:
* Package naming: I'd name this python-mako1.0 with a dot to separate the components of the version number

Comment 5 Matěj Cepl 2015-01-28 18:33:14 UTC
(In reply to Toshio Ernie Kuratomi from comment #4)
> * man page is a warning but up to you if you want to add one.  Personally
> I'd see if the main python-mako packager wants to add one.  I think that
> Debian/Ubuntu has one so they could possibly copy that (or generate it if
> that's all that Debian is doing).

See bug 1186877

> * Invalid source is a problem.  mentioned in previous comment that they seem
> to have moved to:
> https://pypi.python.org/packages/source/M/Mako/Mako-%{version}.tar.gz

Fixed (already in 1.0.1 for python-mako)

> * License tag for the javascript seems outdated, no longer under the GPL.  I
> do see BSD license references though.  So probably needs to be updated to: 
>     # The documentation contains javascript for search which are licensed
> BSD and MIT
>     License: (MIT and Python) and (BSD and GPLv2)

fixed

> * Need to change from unversioned python macros to versioned macros.  Like
> this:

Do I have to? This package will not be used outside of EPEL (I guess), so why should we use non-EPEL names just so that we have to define them?

>  * mako-render needs to be renamed to be parallel installable.  Since we
> don't currently need to worry about python3 here, either of these of these
> would be fine:
>    - mako-render-1.0
>    - mako-render-v1.0

done

> Judgement calls:
> * Package naming: I'd name this python-mako1.0 with a dot to separate the
> components of the version number

done

http://mcepl.fedorapeople.org/tmp/python-mako1.0.spec
http://koji.fedoraproject.org/koji/taskinfo?taskID=8755878 (SRPM https://kojipkgs.fedoraproject.org//work/tasks/5879/8755879/python-mako1.0-1.0.1-2.el6.src.rpm)

Comment 6 Toshio Ernie Kuratomi 2015-01-28 19:08:13 UTC
We talked a bit in IRC about epel6 probably not getting a python3 package so whether the python versioned macros are needed.  I'm willing to approve and the epel steering committee can decide if they want to make that a divergence from Fedora Guidelines for epel6 and below (or even epel7 depending on whether a python34 package gets created there).

Other problems have been fixed.

I noticed two things that can be fixed after import:
* I messed up when I told you what the license line should be.  It should actually be:
  License: (MIT and Python) and (BSD or MIT)

* Should add "This package is a compatibility version of the package for EPEL.
" to the main package %description as well as the doc package %description

Package APPROVED

Comment 7 Matěj Cepl 2015-01-28 19:39:11 UTC
New Package SCM Request
=======================
Package Name: python-mako1.0
Short Description: Mako template library for Python
Upstream URL: http://www.makotemplates.org/
Owners: mcepl
Branches: el6

Comment 8 Gwyn Ciesla 2015-01-28 19:41:13 UTC
WARNING: Ticket is not assigned to anyone.
WARNING: Requested package name python-mako1.0 doesn't match bug summary
python-mako10 

Please correct.

Comment 9 Matěj Cepl 2015-01-29 00:51:55 UTC
New Package SCM Request
=======================
Package Name: python-mako1.0
Short Description: Mako template library for Python
Upstream URL: http://www.makotemplates.org/
Owners: mcepl
Branches: el6

Comment 10 Gwyn Ciesla 2015-01-29 14:37:24 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2015-01-29 15:10:04 UTC
python-mako1.0-1.0.1-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/python-mako1.0-1.0.1-2.el6

Comment 12 Fedora Update System 2015-01-29 19:38:27 UTC
python-mako1.0-1.0.1-2.el6 has been pushed to the Fedora EPEL 6 testing repository.


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