Bug 908114

Summary: Review Request: python-pillow - Python image processing library
Product: [Fedora] Fedora Reporter: Sandro Mani <manisandro>
Component: Package ReviewAssignee: Toshio Ernie Kuratomi <a.badger>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: a.badger, notting, package-review, pf.rhlists, rrakus, terjeros
Target Milestone: ---Flags: a.badger: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-03-19 17:33:26 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Sandro Mani 2013-02-05 17:18:45 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-86.2.fc19.src.rpm
Description: Python image processing library, fork of the Python Imaging Library (PIL)
Fedora Account System Username: smani

See also the feature page at https://fedoraproject.org/wiki/Features/Pillow

Note: Due to the still unpatched python3 (see bug #889784), the python3 subpackages have been disabled. Patched python3 packages are available at http://smani.fedorapeople.org/python3/
Comment 1 Sandro Mani 2013-02-05 17:58:29 EST
Self-review and comments (build including python3 subpackages):

fedora-review output:

Package Review

[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed

- Permissions on files are set properly.
  Note: See rpmlint output
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
** See below **
- Large documentation must go in a -doc subpackage.
  Note: Documentation size is 2600960 bytes in 408 files.
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation
** Done? **

===== MUST items =====

[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
** The unversioned so files are python binary modules **
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
** See comment in spec concerning license **
[?]: %build honors applicable compiler flags or justifies otherwise.
** -fno-strict-aliasing was added for python-imaging-1.1.7-8, without any comment though **
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Fully versioned dependency in subpackages, if present.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in python-
[x]: Package complies to the Packaging Guidelines
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
** There is a hint at the license in the README.rst file **
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 1 files have unknown license. Detailed output of
     licensecheck in /home/sandro/.Data/Desktop/review-python-
[x]: License file installed when any subpackage combination is installed.
** All packages require python{3}-pillow, which has README.rst in doc **
[x]: Package consistently uses macro is (instead of hard-coded directory
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
** Obsoletes python-imaging < 1.7.8, Provides python-imaging **
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

[-]: Python eggs must not download any dependencies during the build process.
[-]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

[!]: Uses parallel make.
** Not doable with setuptools? **
[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
** I guess one could ask upstream for a LICENSE file **
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.
** Some additional patches can be upstreamed (will take care of that) **
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
** See comment at beginning of spec **
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

[x]: Large data in /usr/share should live in a noarch subpackage if package is
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.

Checking: python-pillow-1.7.8-86.2.fc19.x86_64.rpm
python-pillow.x86_64: W: spelling-error %description -l en_US subpackages -> sub packages, sub-packages, prepackages
python-pillow.x86_64: W: spelling-error %description -l en_US devel -> delve, devil, revel
python-pillow.x86_64: W: spelling-error %description -l en_US tk -> kt, t, k
** Ignorable **
python-pillow.x86_64: W: invalid-license PIL
** Ignorable **
python-pillow.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_imagingcms.so 0775L
python-pillow.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_imagingft.so 0775L
python-pillow.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_imaging.so 0775L
python-pillow.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_imagingmath.so 0775L
** I guess this is to be expected for packages with binary python modules **
python-pillow-devel.x86_64: W: invalid-license PIL
** Ignorable **
python-pillow-devel.x86_64: W: no-documentation
** Ignorable **
python-pillow-doc.noarch: W: invalid-license PIL
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/gifmaker.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/player.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/enhancer.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/pilfont.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/thresholder.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/painter.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/explode.py
python-pillow-doc.noarch: W: spurious-executable-perm /usr/share/doc/python-pillow-doc-1.7.8/Scripts/viewer.py
** Ignorable, they are supposed to be executable scripts **
python-pillow-sane.x86_64: W: invalid-license PIL
** Ignorable **
python-pillow-sane.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_sane.so 0775L
** Expected **
python-pillow-tk.x86_64: W: invalid-license PIL
python-pillow-tk.x86_64: W: no-documentation
** Ignorable **
python-pillow-tk.x86_64: E: non-standard-executable-perm /usr/lib64/python2.7/site-packages/_imagingtk.so 0775L
** Expected **
5 packages and 0 specfiles checked; 6 errors, 18 warnings.

rpmbuild -bs python-pillow.spec reports
sh: line 0: fg: no job control
which is due to
%global py3ver %(%{__python3} -c "import sys; print(sys.version[:3])")
on line 3 of the spec. Oddly, the py2ver variant does not cause the warning. I guess this is ignorable though.
Comment 2 Sandro Mani 2013-02-06 15:29:49 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-90.1.fc19.src.rpm

Upstreamed all upstreamable patches.
Comment 3 Terje Røsten 2013-02-07 09:25:12 EST
Please follow: https://fedoraproject.org/wiki/Packaging:SourceURL#Github

Only one BuildRequires on each line and sorted is nice.
Comment 4 Toshio Ernie Kuratomi 2013-02-07 10:57:21 EST
Terje, I'm willing to review this since it's needed for the Pillow Feature.  But if you'd rather do it let me know.  I can start on it this afternoon (about 8 hours from now)
Comment 5 Terje Røsten 2013-02-07 14:17:50 EST
Please continue :-)
Comment 6 Toshio Ernie Kuratomi 2013-02-08 11:51:45 EST
* 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

* You should try to follow the templates for python packages in Fedora:
  * Copying the source directory to %{py3dir} should be done in %prep rather than
  * 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
* 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,
* 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
* 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

* 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
* 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

* 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}
    # 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.
Comment 7 Sandro Mani 2013-02-08 12:01:35 EST
Thanks a lot.
Concerning the -fno-strict-aliasing option, this was added by Roman Rakus in python-imaging 1.1.7-8 in Nov 2012.

Roman, do you remember what the reason for that change was? (CCing Roman on the bug).
Comment 8 Sandro Mani 2013-02-09 18:34:37 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: python-pillow-1.7.8-1.20130210gite09ff61.fc19.src.rpm

- Conform to Python packaging guidelines
- Changed license to MIT, had upstream include a COPYING file
- Added URL for source0.
  Some comments on this: The github guideline suggests using
  However, the name of the file which gets downloaded by visiting that url is actually %{name}-%{commit}.tar.gz
  In my opinion it is better to use
  (note: "tarball" instead of "archive") for the following reasons:
  * The filename is more readable
  * This is also the filename format when one downloads pillow from http://python-imaging.github.com/Pillow/
  Currently I would not use the pypi source + git patch, since the patch would be very large (all the python3 compatibility changes for one)
- Fixed versioning, used recommended snapshot naming
- Fixed obsoletes/provides
- check: The option is to run the check either in the buildroot or in the build folder. The issue with running the check in the build folder is to reliably determine the name of the build folder. For instance, for python2 x86_64, the module is built under $srcfolder/build/lib.linux-$arch-$pyver, with arch x86_64 or i686. I fear that the $arch part may be somewhat fragile.
- Concerning the no-strict-aliasing option, it would be nice to know why the flag was added last November.
- Fixed permissions
- Cosmetic issues fixed
- rpmlint issues fixed
Comment 10 Toshio Kuratomi 2013-02-12 17:48:33 EST
> - Concerning the no-strict-aliasing option, it would be nice to know why the flag was added last November.

Okay -- some spelunking finds this:

$ cd Imaging-1.1.7/libImaging
$ gcc -Wall `python2.7-config --includes` `rpm --eval '%{optflags}'`  -c Quant.c

Quant.c: In function 'rehash_collide':
Quant.c:154:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
Quant.c:154:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
Quant.c: In function 'hash_to_list':
Quant.c:247:4: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]

Those two functions have been updated in pillow to fix that error:

$ cd python-imaging-Pillow-e09ff61/libImaging
$ gcc -Wall `python2.7-config --includes` `rpm --eval '%{optflags}'`  -c Quant.c

Quant.c: In function 'rehash_collide':
Quant.c:154:23: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
Quant.c:154:39: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
Quant.c:154:13: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
Quant.c: In function 'hash_to_list':
Quant.c:247:14: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

Note that these new warnings don't occur on x86, just on x86_64.  I'm thinking they're real bugs... but looking at the source code I'm not sure what needs changing.... It seems like some of them might be saving the address of a pointer when it should be saving the value pointed to by the pointer instead.  It's hard to believe that that kind of bug wouldn't have shown up and been fixed long ago, though....
Comment 11 Toshio Kuratomi 2013-02-12 19:08:58 EST
I've opened an upstream bug against pillow for the pointer being a different size: https://github.com/python-imaging/Pillow/issues/52

We should probably choose one of those strategies and apply a fix for pointer and integer being a different size.
Comment 12 Sandro Mani 2013-02-13 03:03:24 EST
Thanks for your investigation! I will also have a look.
Comment 13 Toshio Ernie Kuratomi 2013-02-13 20:30:40 EST
Regarding github:  it appears that the github urls changed right after we approved those guidelines.  Someone started work on updating the guidelines:


You can add some thoughts to that.  Just be careful that you don't repropose something that was mentioned in the original discussion: https://fedorahosted.org/fpc/ticket/233

Reviewed new spec.  Many things fixed as noted in comment 8.  Remaining issues:

Still to fix:
* -fno-strict-aliasing can now be removed
* Figure out what to do about the pointer to int problem.

* Standard is for the comment about how to get the Source to be right above the Source0: line.

* The new release builds in koji
* New builds don't have any new rpmlint warnings
Comment 14 Roman Rakus 2013-02-14 08:50:25 EST
To answer -fno-strict-aliasing, I don't remember. It was probably to suppress some warning messages. Maybe it fixed something. Better would be to not use this option and fix possible errors.
Comment 15 Sandro Mani 2013-02-18 17:44:43 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-2.20130210gite09ff61.fc19.src.rpm

- Added patch for pointer to int problem
- Removed -fno-strict-aliasing

Cosmetic issue: I'd rather keep the comment at the beginning of the file, since the comment also explains how to set the globals.
Comment 16 Toshio Ernie Kuratomi 2013-02-25 11:27:48 EST
Still to fix:

* CFLAGS="$RPM_OPT_FLAGS"  Should remain even though we've removed -fno-strict-aliasing.
* -fno-strict-aliasing was removed from the python3 section but also needs to be removed from the python2 build section.
* After thinking on it more, I think the comment about how to retrieve the sources really does belong right above the Source0 line.  That doesn't preclude having another comment explaining the %globals if you want.

Everything else fixed.
Comment 17 Sandro Mani 2013-02-25 16:02:01 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-3.20130210gite09ff61.fc19.src.rpm

* Mon Feb 25 2013 Sandro Mani <manisandro@gmail.com> - 1.7.8-3.20130210gite09ff61
- Really remove -fno-strict-aliasing
- Place comment on how to retreive source just above the Source0 line

For the python 3 variant, is there anything I can do? My comments in bug #889784 have so far been ignored, and also a message I posted on fedora-devel some time ago.

Also, I wanted to mention: I didn't find any reference to the python_version and python3_version standard macros in the python packaging guidelines, maybe they could be added to the table at [1]?

[1] https://fedoraproject.org/wiki/Packaging:Python#Macros
Comment 18 Paul Franklin (RHlists) 2013-03-02 01:50:16 EST
I need something like python3-imaging to test the python3
version of gramps 4.0 so if this can provide anything like
that I am all in favor of it!


Comment 19 Sandro Mani 2013-03-05 17:49:45 EST
Spec URL: http://smani.fedorapeople.org/python-pillow.spec
SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-4.20130305git7866759.fc19.src.rpm

* Tue Mar 05 2013 Sandro Mani <manisandro@gmail.com> - 1.7.8-4.20130305git7866759
- Update to latest git snapshot
- 0001-Cast-hash-table-values-to-unsigned-long.patch now upstream
- Pillow-1.7.8-selftest.patch now upstream
Comment 20 Toshio Ernie Kuratomi 2013-03-06 10:49:51 EST
All identified issues have been fixed.  This is APPROVED.

I pinged the python3 bug report last week on behalf of fesco but still no response.
Comment 21 Sandro Mani 2013-03-06 11:00:25 EST

I mentioned the problem concerning the python3 bug yesterday to jreznik, who today directly pinged dmalcom, furthermore according to bkarda the proposed patch is ok, so maybe we will see some activity on that front soonish. If nothing happens until the weekend, I'll push the python2-only variant to rawhide, so that it lands before branching F19.
Comment 22 Sandro Mani 2013-03-06 11:05:24 EST
New Package SCM Request
Package Name: python-pillow
Short Description: Python image processing library
Owners: smani
Comment 23 Gwyn Ciesla 2013-03-06 11:15:54 EST
Git done (by process-git-requests).