Bug 218678
Summary: | Review Request: pybluez - python API for the bluez bluetooth stack | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Will Woods <wwoods> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cr33dog |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
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: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Will Woods
2006-12-06 19:18:51 UTC
Should have mentioned - this is my first package. Be gentle! 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. 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 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. 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 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. Huzzah! Built! |