Bug 980071 - Review Request: python-xpyb - X Python Binding,based on the X C Binding (XCB) library
Review Request: python-xpyb - X Python Binding,based on the X C Binding (XCB)...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1005447
  Show dependency treegraph
 
Reported: 2013-07-01 06:55 EDT by Tomas Dabašinskas
Modified: 2013-09-07 00:55 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-07-21 23:22:36 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
i: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
python-xpyb License Check (3.34 KB, text/plain)
2013-07-06 04:55 EDT, Christopher Meng
no flags Details

  None (edit)
Description Tomas Dabašinskas 2013-07-01 06:55:22 EDT
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-2.fc19.src.rpm
Description: The Python binding for XCB allows the X protocol to be accessed directly from Python. There are two components:

- A Python extension written in C. This exposes XCB-specific objects and
  library functions, as well as providing various base classes used by the
  generated code.
- 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.

Fedora Account System Username: tdabasin
Comment 1 Christopher Meng 2013-07-01 23:13:57 EDT
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
Comment 2 Christopher Meng 2013-07-03 06:17:24 EDT
Hi, please fix the problem ASAP, I'm waiting for your good news.
Comment 3 Tomas Dabašinskas 2013-07-04 08:27:42 EDT
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@redhat.com> - 1.3-2
+* Mon Jul 01 2013 Tomas Dabasinskas <tomas@redhat.com> - 1.3.1-1
 - Minor edits for inclusion to fedora yum repo
 * Sat Apr 20 2013 Huaren Zhong <huaren.zhong@gmail.com> - 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
Comment 4 Mario Blättermann 2013-07-06 03:58:03 EDT
(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}
Comment 5 Christopher Meng 2013-07-06 04:55:03 EDT
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
Comment 6 Christopher Meng 2013-07-06 04:55:47 EDT
Created attachment 769536 [details]
python-xpyb License Check
Comment 7 Michael Schwendt 2013-07-06 13:39:21 EDT
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
Comment 8 Tomas Dabašinskas 2013-07-08 08:54:51 EDT
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@redhat.com> - 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@gmail.com> - 1.3
 - Rebuild for Fedora
 * Mon Apr  8 2013 mlin@suse.com
Comment 9 Christopher Meng 2013-07-08 09:15:08 EDT
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?
Comment 10 Michael Schwendt 2013-07-08 16:09:09 EDT
> .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.
Comment 11 Tomas Dabašinskas 2013-07-09 08:35:56 EDT
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
Comment 12 Christopher Meng 2013-07-09 08:45:21 EDT
APPROVED.
Comment 13 Tomas Dabašinskas 2013-07-09 09:01:58 EDT
New Package SCM Request
=======================
Package Name: python-xpyb
Short Description: Python bindings to XCB
Owners: tdabasin
Branches: f18 f19 el6
InitialCC: tdabasin
Comment 14 Gwyn Ciesla 2013-07-09 09:09:36 EDT
Git done (by process-git-requests).
Comment 15 Tomas Dabašinskas 2013-07-11 08:01:53 EDT
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

Note You need to log in before you can comment on or make changes to this bug.