Bug 980071

Initial look into your spec(only spec, SRPM will later after your changes):

First of all, IMO please cleanup the spec changelog, I know you just copied and edited something, but, I think no need to keep the copyright because you have to change a lot of things still.

1. If you %global src_name xpyb, I think in Source0/%prep/%build you should replace it. Or just drop this %global, you can see that xpyb is only 4 chars, src_name is 8; And you've defined the macro but you don't use it everywhere.

2. I think you can drop Group tag, as Fedora doesn't think it's important now.(optional)

3. Summary tag should be changed to "Python bindings to XCB". This is really simple and you can see that other distros also use it(simple but strong)

4. Why did you keep the %defattr, which is obsoleted from RPM 4.4?

5. subpackage naming should be simple,

%package -n python-xpyb-devel
%description -n python-xpyb-devel

can be replaced by 

%package devel
%description devel

6. Requires:	python-xpyb == %{version} is incorrect IMO, we always use:

Requires:	%{name} = %{version}-%{release}

7. You are not packaging the latest version.

http://xcb.freedesktop.org/dist/xpyb-1.3.1.tar.bz2 is the latest.

8. %description:

"The Python binding requires libxcb.so to work. The additional extension
libraries are not required."

Ah..Do you think we need this?

So, as a summary, please rewrite the spec :D

Hi, please fix the problem ASAP, I'm waiting for your good news.

Christopher, appologies about the delay. Here's the diff of changes I've made:

--- python-xpyb.spec.old        2013-07-01 21:32:02.000000000 +1000
+++ python-xpyb.spec    2013-07-03 05:28:55.356712587 +1000
@@ -1,10 +1,7 @@
-%global src_name xpyb
-
 Name:          python-xpyb
-Version:       1.3
-Release:       2%{?dist}
-Summary:       X Python Binding,based on the X C Binding (XCB) library
-Group:         Development/Libraries
+Version:       1.3.1
+Release:       1%{?dist}
+Summary:       Python bindings to XCB
 License:       Public Domain
 URL:           http://xcb.freedesktop.org
 Source:                http://xcb.freedesktop.org/dist/xpyb-%{version}.tar.bz2
@@ -22,15 +19,11 @@
 - Python modules which are generated directly from the XCB-XML protocol
   descriptions.

-The Python binding requires libxcb.so to work. The additional extension
-libraries are not required.
-
-%package -n python-xpyb-devel
+%package devel
 Summary:       Devel files of xpyb
-Group:         Development/Libraries
-Requires:      python-xpyb == %{version}
+Requires:      %{name} = %{version}-%{release}

-%description -n python-xpyb-devel
+%description devel
 This package provides devel files of xpyb, a Python binding to the
 X Window System protocol via libxcb.

@@ -46,17 +39,15 @@
 %make_install

 %files
-%defattr(-,root,root,-)
 %{python_sitearch}/*
-%doc %{_docdir}/%{src_name}/*
+%doc %{_docdir}/xpyb/*

 %files devel
-%defattr(-,root,root)
-%{_libdir}/pkgconfig/%{src_name}.pc
+%{_libdir}/pkgconfig/xpyb.pc
 %{_includedir}/*.h

 %changelog
-* Mon Jul 01 2013 Tomas Dabasinskas <tomas> - 1.3-2
+* Mon Jul 01 2013 Tomas Dabasinskas <tomas> - 1.3.1-1
 - Minor edits for inclusion to fedora yum repo
 * Sat Apr 20 2013 Huaren Zhong <huaren.zhong> - 1.3
 - Rebuild for Fedora

If you don't mind I'd like to keep the original rpm author names. I am using somebody else's work and I want to be fair, if you insist I remove the entries I can do that.

Many thanks for your time on this review.

Spec URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb.spec
SRPM URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb-1.3.1-1.fc19.src.rpm

(In reply to Christopher Meng from comment #1)
> 6. Requires:	python-xpyb == %{version} is incorrect IMO, we always use:
> 
> Requires:	%{name} = %{version}-%{release}

This package isn't noarch, so we have to add the isa tag:

Requires: %{name}%{?_isa} = %{version}-%{release}

Better than before:

1. I've found that rpmlint warned of license mixture:

Checking patched sources after %prep for licenses. Licenses found:
     "GPL (v2 or later)", "Unknown or generated". 39 files have unknown
     license.

I will upload an attachment of licensecheck result.

2. Same like Mario, I think this package is not noarch, so...

3. Upstream install docs with INSTALL file, I think we should drop it, consider an upstream fix and just patch for a while.

Once fixed I'll give + of fedora-review

Created attachment 769536 [details]
python-xpyb License Check

There are a few more packaging mistakes:


* /usr/include/xpyb.h includes Python and xcb headers, so python-xpyb-devel is missing dependencies


* the pkgconfig file links with libxcb, so python-xpyb-devel is missing dependencies


* The package includes an unneeded .la file in the Python module dir:

| Libtool archives, foo.la files, should not be included.
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> %doc %{_docdir}/xpyb/*

Files in %_docdir are marked %doc automatically.
Plus https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Hi all many thanks for the feedback.

Mario,
I have updated the requires to include %{?_isa}

Christopher,
The license on pipy site says it's Public Domain https://pypi.python.org/pypi/xpyb,
I'm not sure if I'm defining correctly as rpmlint says it doesn't know that license type. 

Michael,
I have included devel subpackage requires for libxcb-devel and python2-devel, libxcb is required for libxcb-devel, should I say requires: libxcb anyway?
doc dir, and one other dir updated


Can anyone please tell me how do I go about removing xcb.la and INSTALL files? I can't find anything on the wiki

Many thanks for your time everyone.

Spec URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb.spec
SRPM URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb-1.3.1-1.fc19.src.rpm


--- python-xpyb.spec.old        2013-07-06 19:31:55.714203065 +1000
+++ python-xpyb.spec    2013-07-06 20:12:14.372602110 +1000
@@ -21,7 +21,9 @@

 %package devel
 Summary:       Devel files of xpyb
-Requires:      %{name} = %{version}-%{release}
+Requires:      %{name}%{?_isa} = %{version}-%{release}
+Requires:      libxcb-devel
+Requires:      python2-devel

 %description devel
 This package provides devel files of xpyb, a Python binding to the
@@ -39,8 +41,8 @@
 %make_install

 %files
-%{python_sitearch}/*
-%doc %{_docdir}/xpyb/*
+%{python_sitearch}/xcb/
+%{_docdir}/xpyb/

 %files devel
 %{_libdir}/pkgconfig/xpyb.pc
@@ -48,7 +50,7 @@

 %changelog
 * Mon Jul 01 2013 Tomas Dabasinskas <tomas> - 1.3.1-1
-- Minor edits for inclusion to fedora yum repo
+- Edits for inclusion to fedora yum repo
 * Sat Apr 20 2013 Huaren Zhong <huaren.zhong> - 1.3
 - Rebuild for Fedora
 * Mon Apr  8 2013 mlin

Ah...

I'm not sure if la files are needed. I've tried configure with --disable-static, but it's useless.

You can just use find to delete them:

find %{buildroot} -name '*.la' -exec rm -f {} ';'

And

find %{buildroot} -name 'INSTALL' -exec rm -f {} ';'

BTW, I've noticed a --enable-selinux option ,is it needed?

> .la files

Nobody seems to know why "libtool archives" are covered by the Static Library packaging guidelines section (as linked in comment 7). Those .la files cause more trouble during shared linking (-> extra inter-dependencies in generated .la files):
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

Simply delete the .la files in the %buildroot. There are only few programs that use an old libtool dlopen method to load .la files at run-time instead of loading .so files. Python loads C based modules via the .so libs.


> INSTALL files

That's covered here:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
|
| Irrelevant documentation include build instructions, the omnipresent
| INSTALL file containing generic build instructions, [...]

Which means that if the INSTALL file is a copy of /usr/share/autoconf/INSTALL (as generated by Autoconf/Automake) or contains similar instructions referring to building from source code, that's pretty much uninteresting to users of Fedora's prebuilt RPM packages. The contents of such files have caused confusion before, so not packaging them is the better choice.

Hi,
Thanks for the tip on selinux Christopher, I noticed there was a request to enable it on BZ580732 so I did that, I verified it by checking outputs from rpm -qpl and I see 3 extra files now:
+/usr/lib64/python2.7/site-packages/xcb/xselinux.py
+/usr/lib64/python2.7/site-packages/xcb/xselinux.pyc
+/usr/lib64/python2.7/site-packages/xcb/xselinux.pyo

I've also added find and delete *.la and INSTALL files.

I've updated the spec:

--- python-xpyb.spec.old        2013-07-07 19:05:53.053592649 +1000
+++ python-xpyb.spec    2013-07-07 19:25:24.206440154 +1000
@@ -33,12 +33,17 @@
 %setup -q -n xpyb-%{version}

 %build
-%configure --docdir=%{_docdir}/xpyb
+%configure --docdir=%{_docdir}/xpyb \
+       --enable-selinux

 make %{?_smp_mflags}

 %install
 %make_install
+# Delete static lib
+find %{buildroot} -name '*.la' -exec rm -f {} ';'
+# Delete INSTALL doc
+find %{buildroot} -name 'INSTALL' -exec rm -f {} ';'

 %files
 %{python_sitearch}/xcb/


Spec URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb.spec
SRPM URL: http://people.fedoraproject.org/~tdabasin/python-xpyb/python-xpyb-1.3.1-1.fc19.src.rpm

APPROVED.

New Package SCM Request
=======================
Package Name: python-xpyb
Short Description: Python bindings to XCB
Owners: tdabasin
Branches: f18 f19 el6
InitialCC: tdabasin

Git done (by process-git-requests).

Guys, thanks for your help with the review, package is built, but it's only a part of the final goal. If anyone is interested working on helping bring qtile window manager ( www.qtile.org ) to fedora please get in touch, I have put up change proposal here: https://fedoraproject.org/wiki/Changes/Qtile please write your names in "Scope" section. 

Many thanks,
Tomas