Bug 788080

Hi Matthias,

I am not a package maintainer so I can only make an informal review, here are some notes:

* build is recommended to be: CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build
* is there a specific reason to have -O1 in the install command?
* I think you should be more explicit in files section to detect when egg build fail with something like:
%{python_sitelib}/%{pkgname}-%{version}-py?.?.egg-info
%{python_sitelib}/%{pkgname}/
* probably you should explicitly set executable on the two scripts in bindir
* you should also provide man pages for the two bindir scripts as it is recommended

It builds in mock and the rest looks fine to me.

Hi Massimo,

thank you for your annotations.

* build is only recommended to set rpm-flags on arch-dependent packages. This one is noarch.

* there's no specific reason for -O1 on install. It simply comes from rpmdev-newspec (it's a given template). Install does not compile something, but just copies the files into buildroot. 

* files: section: it's up to the packager, how to handle those files. Being stricter would be cleaner. 

* man-page: you're right, I'd love to provide man-pages; sadly upstream does not provide man-pages. It's just a *should*, not a *MUST*. 


applying files-cleanup:
@@ -2,7 +2,7 @@
 
 Name:           python-xhtml2pdf
 Version:        0.0.3
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        HTML/CSS to PDF converter based on Python
 
 # licensed under ASL 2.0 and BSD-License
@@ -55,11 +55,15 @@
  
 %files
 %doc LICENSE.txt README.rst VERSION.txt MANIFEST.in
-%{python_sitelib}/*
+%{python_sitelib}/%{pkgname}-%{version}-py?.?.egg-info
+%{python_sitelib}/%{pkgname}/
 %{_bindir}/pisa
 %{_bindir}/xhtml2pdf
 
 
 %changelog
+* Mon Feb 13 2012 Matthias Runge <mrunge> - 0.0.3-2
+- be more explicit in %%files-section
+
 * Tue Feb 07 2012 Matthias Runge <mrunge> - 0.0.3-1
 - initial packaging


Updated SRPM: http://www.matthias-runge.de/fedora/python-xhtml2pdf-0.0.3-2.fc16.src.rpm
Updated SPEC: http://www.matthias-runge.de/fedora/python-xhtml2pdf.spec

I will take this review

Looking good - just a couple of things. I've done the formal review, but just need to run these by you.

1. rpmlint on the built package:
python-xhtml2pdf.noarch: E: explicit-lib-dependency python-html5lib
python-xhtml2pdf.noarch: E: explicit-lib-dependency python-httplib2
Can you remove these?

2. Just for completeness use your %{pkgname} var in the %files section

3. Build is failing on koji in the %test section:

http://koji.fedoraproject.org/koji/taskinfo?taskID=3800044 - it is try to pull down the reportlab egg

4. Just a note: it seems to be bundling some of the reportlab source.
/usr/lib64/python2.7/site-packages/reportlab/platypus/paragraph.py
vs.
xhtml2pdf/reportlab_paragraph.py
However it seems that this has been heavily modified, quite well annotated and doesn't breach the reportlab license (BSD) so no blocker.

Thank you for your remarks, I tried to fix them (if possible)

Those explicit dependencies are, imho falso positives from rpmlint. If left out, the package does not work, because of broken imports.

[mrunge@mrungexp SPECS]$ diff -u python-xhtml2pdf.spec.2 python-xhtml2pdf.spec
--- python-xhtml2pdf.spec.2	2012-02-13 11:58:38.000000000 +0100
+++ python-xhtml2pdf.spec	2012-02-20 10:44:32.734080127 +0100
@@ -2,7 +2,7 @@
 
 Name:           python-xhtml2pdf
 Version:        0.0.3
-Release:        2%{?dist}
+Release:        3%{?dist}
 Summary:        HTML/CSS to PDF converter based on Python
 
 # licensed under ASL 2.0 and BSD-License
@@ -19,9 +19,18 @@
 BuildRequires:  python-nose
 BuildRequires:  pyPdf
 
+# required for testing:
+BuildRequires:       python-reportlab
+BuildRequires:       python-imaging
+BuildRequires:       python-html5lib
+BuildRequires:       python-httplib2
+
 Requires:       pyPdf
 Requires:       python-reportlab
 Requires:       python-imaging
+
+# removing will create rpmlint-false positive
+# html5lib and httplib both are requires to work
 Requires:       python-html5lib
 Requires:       python-httplib2
 
@@ -58,10 +67,14 @@
 %{python_sitelib}/%{pkgname}-%{version}-py?.?.egg-info
 %{python_sitelib}/%{pkgname}/
 %{_bindir}/pisa
-%{_bindir}/xhtml2pdf
+%{_bindir}/%{pkgname}
 
 
 %changelog
+* Mon Feb 20 2012 Matthias Runge <mrunge> - 0.0.3-3
+- fix tests to complete testing
+- add comment about required html5lib and httplib2
+
 * Mon Feb 13 2012 Matthias Runge <mrunge> - 0.0.3-2
 - be more explicit in %%files-section
 

Package now builds in koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3804269
(sorry for this, I should have done this earlier)

updated SPEC: http://www.matthias-runge.de/fedora/python-xhtml2pdf.spec
updated SRPM: http://www.matthias-runge.de/fedora/python-xhtml2pdf-0.0.3-3.fc16.src.rpm

This package is APPROVED

[-] N/A
[+] Good
[!] Attention
[N] Not performed


Required
========

[+] named according to the Package Naming Guidelines 
[+] The spec file name must match the base package %{name}, in the format
%{name}.spec 
[+] Meet the Packaging Guidelines
[+] Be licensed with a Fedora approved license and meet the Licensing
Guidelines 
[+] The License field in the package spec file must match the actual license 
[+] License file must be included in %doc
[+] The spec file must be written in American English
[+] The spec file for the package MUST be legible
[ ] The sources used to build the package must match the upstream source
md5sium 13b0d6059b72c994473fddfa7a528451 OK
[+] Successfully compile and build into binary rpms on at least one primary
architecture
[+] Proper use of ExcludeArch 
[+] All build dependencies must be listed in BuildRequires
[-] The spec file MUST handle locales properly
[-] Shared library files (not just symlinks) in any of the dynamic linker's
default paths, must call ldconfig in %post and %postun
[-] Packages must NOT bundle copies of system libraries
[-] If the package is designed to be relocatable, the packager must state this
fact in the request for review, along with the rationalization for relocation
of that specific package
[+] A package must own all directories that it creates
directories under this
[+] A Fedora package must not list a file more than once in the spec file's
%files listings
[+] Permissions on files must be set properly. %defattr(...) no longer required
[+] Each package must consistently use macros
[+] The package must contain code, or permissable content
[-] Large documentation files must go in a -doc subpackage

[+] If a package includes something as %doc, it must not affect the runtime of
the application
[-] Header files must be in a -devel package
[-] Static libraries must be in a -static package
[-] library files that end in .so (without suffix) must go in a -devel package
[-] devel packages must require the base package using a fully versioned
dependency
[-] Packages must NOT contain any .la libtool archives
[-] GUI apps must include a %{name}.desktop file, properly installed with
desktop-file-install in the %install section 
[+] Packages must not own files or directories already owned by other packages
[-] All filenames in rpm packages must be valid UTF-8

[+] Has BuildRequires: python2-devel and/or python3-devel
[+] Defines and uses %{python_sitelib} or %{python_sitearch}:
[+] Has BuildRequires: python-setuptools-devel

[+] Python eggs must be built from source. They cannot simply drop an egg from upstream into the proper directory.
[+] Python eggs must not download any dependencies during the build process.
[+] If egg-info files are generated by the modules build scripts they must be included in the package.
[-] When building a compat package, it must install using easy_install -m so it won't conflict with the main package.
[-] When building multiple versions (for a compat package) one of the packages must contain a default version that is usable via "import MODULE" with no prior setup.
[-] A package which is used by another package via an egg interface should provide egg info. 
[!] Requires OK

[+] Egg install:
%install
%{__python} setup.py install --skip-build --root $RPM_BUILD_ROOT 

Should Items
============
[-] the packager SHOULD query upstream for any missing license text files to
include it
[-] Non-English language support for description and summary sections in the
package spec if available
[+] The reviewer should test that the package builds in mock
[N] The package should compile and build into binary rpms on all supported
architectures
[N] The reviewer should test that the package functions as described
tests OK
[-] If scriptlets are used, those scriptlets must be sane
[-] Usually, subpackages other than devel should require the base package using
a fully versioned dependency
[-] The placement of pkgconfig(.pc) should usually be placed in a -devel pkg
[-] If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself
[-] Should contain man pages for binaries/scripts
None in source

Thanks for the review!

New Package SCM Request
=======================
Package Name: python-xhtml2pdf
Short Description: HTML/CSS to PDF converter based on Python
Owners: mrunge
Branches: f16 f17 el6 devel

Git done (by process-git-requests).

python-xhtml2pdf-0.0.3-3.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/python-xhtml2pdf-0.0.3-3.fc16

python-xhtml2pdf-0.0.3-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/python-xhtml2pdf-0.0.3-3.fc17

python-xhtml2pdf-0.0.3-3.fc17 has been pushed to the Fedora 17 testing repository.

python-xhtml2pdf-0.0.3-3.fc17 has been pushed to the Fedora 17 stable repository.

python-xhtml2pdf-0.0.3-3.fc16 has been pushed to the Fedora 16 stable repository.

python-xhtml2pdf-0.0.3-3.fc16 has been pushed to the Fedora 16 stable repository.