Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=908114 Toshio Ernie Kuratomi <a.badger@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@xxxxxxxxxxxxxxxxx |a.badger@xxxxxxxxx --- Comment #6 from Toshio Ernie Kuratomi <a.badger@xxxxxxxxx> --- 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 _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review