Bug 218678 - Review Request: pybluez - python API for the bluez bluetooth stack
Summary: Review Request: pybluez - python API for the bluez bluetooth stack
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-12-06 19:18 UTC by Will Woods
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

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


Attachments (Terms of Use)

Description Will Woods 2006-12-06 19:18:51 UTC
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 19:49:35 UTC
Should have mentioned - this is my first package. Be gentle!

Comment 2 Chris Mohler 2006-12-15 02:12:25 UTC
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 22:07:08 UTC
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 23:32:38 UTC
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 18:28:34 UTC
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 23:30:12 UTC
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 21:58:03 UTC
Huzzah! Built!


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