Bug 218678 - Review Request: pybluez - python API for the bluez bluetooth stack
Review Request: pybluez - python API for the bluez bluetooth stack
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-06 14:18 EST by Will Woods
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-05 16:58:03 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Will Woods 2006-12-06 14:18:51 EST
Spec URL: http://homepage.mac.com/wgwoods/fedora/pybluez.spec
SRPM URL: http://homepage.mac.com/wgwoods/fedora/pybluez-0.9.1-1.src.rpm
Description: PyBluez is an effort to create python wrappers around system bluetooth resources to allow Python developers to easily and quickly create Bluetooth applications.
Comment 1 Will Woods 2006-12-06 14:49:35 EST
Should have mentioned - this is my first package. Be gentle!
Comment 2 Chris Mohler 2006-12-14 21:12:25 EST
This is my first review.  I cannot officially review the package - this is just
a "pre-review".

The SRPM passes rpmlint.

It builds in mock fc6-i386, which is a good sign.

I notice a couple of issues with the spec file, however:
* You need to remove the appropriate line (and the comment) from the 
  beginning of the spec file.  At appears that this is an arch-specific(?)
  package, so you need to remove the '%{!?python_sitelib' line.

* I think you should remove the comment from the Build section, and
  also the one from above '%{python_sitearch}/*'

According to
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b7d622f4bb245300199c6a33128acce5fb453213:
* The changelog items should begin with a dash.



Comment 3 Will Woods 2006-12-15 17:07:08 EST
Ah, good suggestions. I've rebuilt the package with them.

I'm not sure whether I should just replace the SRPM/specfile or bump the release
number and give the new spec a different name. I'll do the latter for now.

Spec URL: http://homepage.mac.com/wgwoods/fedora/pybluez-0.9.1-2.spec
SRPM URL: http://homepage.mac.com/wgwoods/fedora/pybluez-0.9.1-2.src.rpm
Comment 4 Kevin Fenzi 2006-12-26 18:32:38 EST
Hey Will. 

I would be happy to move this review forward and look at sponsoring you. 
Do you have any other packages ready to review right now? It helps to see a few
packages to know when someone knows the ropes and is ready to be sponsored. 

Anyhow, here is a review of this package: 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
3e7e951ea4e8433f3b80ef8c14d99c28  pybluez-src-0.9.1.tar.gz
3e7e951ea4e8433f3b80ef8c14d99c28  pybluez-src-0.9.1.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock. 
i386/x86_64 - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Why the:
BuildArch:          i386 x86_64 ppc
line? Does it not work on other arches?

2. In build you have:
# Remove CFLAGS=... for noarch packages (unneeded)
CFLAGS="$RPM_OPT_FLAGS" %{__python} setup.py build

why are you passing them in if you don't need them?
Perhaps that line should be:
%{__python} setup.py build

3. rpmlint says:
W: pybluez incoherent-version-in-changelog -0.9.1-2 0.9.1-2.fc7

There is a - in there that shouldn't be.   
Also, you don't need to add dist tags normally to changelog versions.

W: pybluez doc-file-dependency
/usr/share/doc/pybluez-0.9.1/examples/bluezchat/bluezchat.py /usr/bin/python

The bluezchat.py example should be made mode 644 like all the other examples.

4. The package provides:
_bluetooth.so
Which is kinda ugly, but not a blocker I guess.
Comment 5 Will Woods 2006-12-29 13:28:34 EST
Thanks very much for the help! I don't currently have any other packages ready to review, but if you 
absolutely need to see some example specfiles I could dig some up from some internal packages I've 
made.

As for the issues you mentioned:

1) Heh! I just wasn't sure what to do with the BuildArch line, so I put the three arches that Fedora 
currently supports. I'll remove it so this package can be all-arch.

2) This package is *not* noarch, hence the CFLAGS part *is* needed. I've removed the comment to 
avoid confusion.

3) Whoops, there should have been a space between that initial dash and the version. Fixed.
Also, bluezchat.py is now chmod'd a-x before installation, which makes that rpmlint warning go away.

4) Why is that ugly? Isn't it normal for python modules which contain binary extensions to provide 
the .so file for that extension?

Here are the updated files:

Spec URL: http://homepage.mac.com/wgwoods/fedora/pybluez-0.9.1-3.spec
SRPM URL: http://homepage.mac.com/wgwoods/fedora/pybluez-0.9.1-3.src.rpm
Comment 6 Kevin Fenzi 2006-12-29 18:30:12 EST
1, 2, 3 all look fixed up in the package in comment #5. 

On item 4: rpm happily looks for .so files anywhere in the package and then
automagically "Provides" them in the rpm namespace. I think that rpm should
instead only do this with .so files when they are in the standard ldconfig
directories. The only thing that can really use the _bluetooth.so thats provided
here is this package, so the provides just adds to the rpm provides namespace. 
It's not a blocker though, since it doesn't hurt anything. If the .so here was
bluetooth.so that would be a problem... since bluez-libs-devel already provides
that. Hopefully that makes some sense to someone aside from me. ;) 

I don't see any other blockers here... I will go ahead and APPROVE this package
and sponsor you. 

You can continue the process from here: 
http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907

Don't forget to close this review request NEXTRELASE once everything has been
imported and built. 
Comment 7 Will Woods 2007-01-05 16:58:03 EST
Huzzah! Built!

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