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/
Self-review and comments (build including python3 subpackages): ------- fedora-review output: Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed Issues: ======= - 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 ===== C/C++: [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. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. ** 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- pillow-doc [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- pillow/licensecheck.txt [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 names). [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 work. [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 %{name}.spec. [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: [-]: 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 ===== Generic: [!]: 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 architectures. [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 $RPM_BUILD_ROOT) [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 ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [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. Rpmlint ------- Checking: python-pillow-1.7.8-86.2.fc19.x86_64.rpm python-pillow-devel-1.7.8-86.2.fc19.x86_64.rpm python-pillow-doc-1.7.8-86.2.fc19.noarch.rpm python-pillow-sane-1.7.8-86.2.fc19.x86_64.rpm python-pillow-tk-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. ------- Other: 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.
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.
Please follow: https://fedoraproject.org/wiki/Packaging:SourceURL#Github Only one BuildRequires on each line and sorted is nice.
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)
Please continue :-)
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.
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).
Spec URL: http://smani.fedorapeople.org/python-pillow.spec SRPM URL: python-pillow-1.7.8-1.20130210gite09ff61.fc19.src.rpm Changes: - 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 [...]/archive/$commit/$name-$version-$shortcommit.tar.gz 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 [...]/tarball/$commit/$name-$version-$ahead-g$shortcommit.tar.gz (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
Spec URL: http://smani.fedorapeople.org/python-pillow.spec SRPM URL: http://smani.fedorapeople.org/python-pillow-1.7.8-1.20130210gite09ff61.fc19.src.rpm (SRPM URL...)
> - 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....
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.
Thanks for your investigation! I will also have a look.
Regarding github: it appears that the github urls changed right after we approved those guidelines. Someone started work on updating the guidelines: https://fedorahosted.org/fpc/ticket/252 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. Cosmetic: * Standard is for the comment about how to get the Source to be right above the Source0: line. Good: * The new release builds in koji * New builds don't have any new rpmlint warnings
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.
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 Changed: - 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.
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.
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> - 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
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! +1 Thanks.
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> - 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
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.
Thanks! 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.
New Package SCM Request ======================= Package Name: python-pillow Short Description: Python image processing library Owners: smani Branches: InitialCC:
Git done (by process-git-requests).