[Bug 908114] Review Request: python-pillow - Python image processing library

[bugzilla at redhat.com]

Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=908114

Toshio Ernie Kuratomi <a.badger at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|nobody at fedoraproject.org    |a.badger at gmail.com

--- Comment #6 from Toshio Ernie Kuratomi <a.badger at gmail.com> ---
GOOD:
* Naming is fine.  The package's module is PIL but upstream's project name is
  pillow.  Because this is a fork from the original PIL, it's okay to make use
  of the python-pillow name in this instance.
* Spec file is legible
* No locale files to be taken care of.
* No elf libraries
* No bundled libraries
* Not designed to be relocatable
* builds in koji
* owns all directories it creates
* Code and permissible content (documentation)
* Exensive API reference docs are in a separate subpackage
* Nothing in %doc affects the package runtime
* Does not own files more than once except in the python3 vs python2 main
  packages.  This is allowed as there is no dependency between the two.
* Macros used consistently but see the note about following the template for
  python modules more closely below.  That will change some of hte macro names 
* Not a GUI app
* Does not own files or dirs that belong to other packages
* Filenames valid UTF-8
* No scriptlets

NEEDSWORK:
* You should try to follow the templates for python packages in Fedora:
  http://fedoraproject.org/wiki/Packaging:Python
  * Copying the source directory to %{py3dir} should be done in %prep rather
than
    %build
  * enablepy3 => with_python3
  * py2ver and py3ver have standard macros, python_version and python3_version
* License: PIL is not a valid license short text for Fedora.  However, looking
  at the license text and the python-imaging package, it appears that this is
MIT.
* Sources don't have a way to check them against upstream:
  * You could build from the tarball on pypi http://pypi.python.org/pypi/Pillow
    (Applying a patch taken from the git repository if necessary) to solve
this.
  * You could instead continue to use the github checkout but if so you should
    use the github guidelines that terje mentions to construct it and include a
    comment explaining how someone else can retrieve the same tarball from
    github.http://fedoraproject.org/wiki/Packaging:SourceURL#Github
* Versioning is incorrect.  This is a post-release snapshot so it should be
  something like:
  Release: 1.%{ahead}%{dist}
  (Our Release number, "1", comes before anything extra added by upstream,
  "%{ahead}".)
* You should be Obsoleting the python-imaging packages at version
  1.1.7-10%{?dist} (the last build) rather than 1.7.8-1 (the version of the
  first python-pillow package).  Because of %{?dist} you should increment the
  release by one so that it is higher than the release+dist tag.  Since this
  probably won't replace python-imaging until after the mass rebuild, the
  release you obsolete will probably go up another number for that.  So
  Something like this:
  Obsoletes: python-imaging <= 1.1.7-12
* %check -> Why don't you just run %check in the directory you build the module
  in rather than in the $RPM_BUILD_ROOT.  It's non-standard to run the checks
  in the BUILD_ROOT and it means that you have to both link the test data and
  script there and then remove the links afterwards.  Seems like it would be
  cleaner if nothing outside of %install touched the $RPM_BUILD_ROOT
  (According to the comments, this would also allow you to drop the patch)
* One subpackage requires the wrong base package: python3-pillow-doc currently
  Requires: %{name} = %{version}-%{release}.
  That should be %{name3} = %{version}-%{release}
* You should contact upstream about including a copy of the license file in the
  tarball
* Why are you using -fno-strict-aliasing with CFLAGS? (Note: don't just take
what the python-imaging package did as good... it was reviewed a long time ago
and things may have changed ;-)
* Permissions on the scripts in %doc are incorrect.  Everything in %doc needs
  to be non-executable.  You are changing this in the spec file so you should
  be able to simply remove the chmod 755 Scripts/*.py line

COSMETIC:
* BuildRequires: python-devel => BuildRequires: python2-devel
* Having separate lines for each BuildRequires is also easier to read
* Separating python2 and python3 deps onto separate lines is also good for
  organization
* Directories in the %files section like %{py2_incdir}/Imaging should have a
  trailing slash.  This just tells future maintainers that this is
  intentionally including the directory and everything inside it (It doesn't
  affect rpm's treatment of the directory).
* In setup.py install -O1 you can leave off the -O1


RPMLINT:
* python-pillow.i686: W: spelling-error %description -l en_US subpackages ->
sub packages, sub-packages, prepackages
  - All noted spelling warnings are false positives
* python-pillow.i686: W: invalid-license PIL
  - Noted above as a mustfix
* python-pillow.src:10: W: macro-in-comment %{version}
  - These need to be fixed.  rpm interprets macros even if they're in comments.
 So change things like:
    # This is the %{version}
    to:
    # This is the %%{version}
* python-pillow.src: W: invalid-url Source0:
python-imaging-Pillow-1.7.8-90-gcb4f0f2.tar.gz
  - Noted above that this could use the pypi url and the actual release or the
    github guidelines and a comment in the spec file if a snapshot is needed
* python-pillow-devel.i686: W: no-documentation
  - Ignorable.  The base package (which this depends on) has README type docs
    and there is a -docs subpackage for the rest
* python-pillow-doc.noarch: W: spurious-executable-perm
/usr/share/doc/python-pillow-doc-1.7.8/Scripts/gifmaker.py
  - Noted above -- things in %doc should not be executable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=NZEv8y29Vt&a=cc_unsubscribe