Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 217066 (python-gpod-review)
Summary: | Review Request: python-gpod - A python module to access iPod content | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Todd Zullinger <tmz> |
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 | ||
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: | 2006-12-05 01:44:04 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, 219059 |
Description
Todd Zullinger
2006-11-23 16:35:29 UTC
Bah, I found 2 missing BuildRequires: gettext and perl(XML::Parser). I also removed pkgconfig as gtk2-devel already pulls it in. Spec URL: http://pobox.com/~tmz/fedora/python-gpod.spec SRPM URL: http://pobox.com/~tmz/fedora/python-gpod-0.4.0-2.fc7.src.rpm Since I cannot sponsor the requester, I can not do the formal review of this package. Below I provide my informal review of python-gpod-0.4.0-2.fc7.src.rpm that the sponsering reviewer can look over. I don't see any blockers, see my comments about non-critical specfile style after my review checklist. I will be attempting to test this binding with editted versions of the example scripts provideed in the %docs section. GOOD: Builds in mock against fedora-development-i386 target GOOD: rpmlint runs clean against python-gpod-0.4.0-2.fc7.i386.rpm GOOD: Follows naming guidelines for python packaging. GOOD: the spec file name matches the base package %{name}, in the format %{name}.spec GOOD: the package is licensed LGPL GOOD: the License field matches the actual license. GOOD: COPYING file is included in %doc GOOD: Spec file is written in American English GOOD: Spec file is reasonably legible GOOD: Spec URL points to correct upstream project page GOOD: Included source tarball matches upstream source listed in spec SOURCE0 md5sum: e427e0409b0cb2d7e76b17915b1396fa libgpod-0.4.0.tar.gz GOOD: Locales are not handled in this package. The Locale information is handled in the lingpod package in Core. This package just handled the python bindings which are not currently packaged by Core. GOOD: no shared libraries in linkers default path. GOOD: no request for relocatable packaging GOOD: Package owns all directories it creates. GOOD: No duplicate files in %file GOOD: Permissions are set correctly GOOD: %clean section GOOD: Consistent use of $RPM_BUILD_ROOT GOOD: No significant 'content' in package GOOD: no -docs packagable material GOOD: %docs section holds non-critical examples and COPYING file GOOD: no -devel packagable material GOOD: no .la files included GOOD: no gui applications included GOOD: no duplicate ownership of payload files according to repoquery GOOD: BuildRequires section in spec looks good, SPECFILE NOTES: the sed script to strip the #!/usr/bin/env python line from gtkpod is fine and is technically correct since gtkpod.py is not meant to be run as a standalone executable. Would this be better done as a patch file? The sed script is really simple so I'm not inclined to say this must be done as a patch file. Its a technically correct but inherently cosmetic change. The multiple rm statements in the %install section serve the purpose of excluding files provided in the Core libgpod package. This could be done with a set of %exclude statements in the %files section, but I don't think there is a requirement to choose either style over the other. -jef the module itself appears to work just fine. The toy examnples that come in the package appear to work. These bindings don't appear work with the listen application, which assumes the 0.3.2 python bindings, but we have to start somewhere. -jef Thanks Jef. I understand the points about using sed and rm and I did consider the alternatives. I'm not tied to either method so if there's a preference to do it one way or the other, I'll change it to be more consistant with other FE packages. My reasoning went like this: Both the sed removal of the bang in gtkpod.py and the chmod of the example scripts were needed to fix things rpmlint would complain about, so it seemed better to do them both in the same place than to put one into a patch that others would have to check out separately. Using rm instead of %excludes seems cleaner looking to me since it's such a wholesale removal. I did use the info from the %files section of the core libgpod package, so perhaps it'd be just as correct to modify those entries into %excludes. Like I said, I'm not tied to either method so if there's a preference for FE packages I'll change to match that. I've not used listen but if I get some time to play with it I'll see if I can locate what things are broken that are preventing it from working (and whether it's in the bindings or listen). The API changes in libgpod weren't major, so any changes that might need made to listen are probably pretty minor. Upstream may even have something in their development that works with 0.4.x already (though you may have already checked that). Todd, with the upcoming merging of Core & Extras does it make sense to create a separate package for the python bindings? I don't know what the timeframe is for that merging. It would mean that there are no python bindings for libgpod in FC5 and FC6, wouldn't it? I got the impression from from of the other bugs and threads referenced in those bugs on f-maintainers and f-e-l that there were folks who wanted to make use of these bindings now. Even after the merger, might it not be a good thing to have the python bindings packaged separately? If so, then when things get merged the specfile for libgpod can just add the BRs for the python bindings and add it as a subpackage. I'd already planned to keep close watch on libgpod in core and work with AlexL on any issues that might come up. Incidentally, if it's agreeable that the bindings should go into a subpacakge once core and extras merge then the pacakge name may be better set to libgpod-python. This is the name I've used in my own personal packaging of libgpod. I used python-gpod here as it seemed to better follow the package naming guidelines for python modules. But using libgpod-python would make it simpler to just add a -python subpackage to the main libgpod SRPM once the merge happens and the required eyeD3 module is available irrespective of which repo it's in. Hey Todd. After reading through all this saga it all sounds reasonable to me... I am going to do some quick builds and make sure I don't see any last minute gotchas, but barring that I will be willing to sponsor you to get this moving forward. I will add another comment in a while here if everything looks good. Thanks to Jef for pushing this all forward... Thanks Kevin. If you have any thoughts on changing the package name to libgpod-python and whether that'd be a good thing in case we manage to merge this into the main libgpod package if/when core and extras merge, please let me know. Of the packages I can find that have python modules as sub packages, nearly all of them use the -python naming convention. (And thanks again to Jef too for the prodding on many fronts.) I think the python-gpod name matches up with all the other python bindings in extras. I would leave it at that for now... I am running into a build issue on x86_64 however... it's installing the _gpod.so library under /usr/lib and not /usr/lib64... /usr/bin/install -c _gpod.so /var/tmp/python-gpod-0.4.0-2.fc7-root-mockbuild/usr/lib/python2.4/site-packages/gpod/_gpod.so I'd be happy to provide access to my x86_64 test machine if you like, just email me your ssh key in private email. A key is on its way. Thanks for the offer and the testing. Okay, after poking a little it seems that $(pythondir) should be $(pyexecdir) in bindings/python/Makefile.{ac,in}. I get double blame for missing that since I submitted the broken auto-stuff in Makefile.am upstream. I've put a new source and spec file up that adds a patch to correct this. I'll submit this upstream for inclusion in the next release as well. (I'm surpised no one noticed this before on any of the libgpod lists.) Spec URL: http://pobox.com/~tmz/fedora/python-gpod.spec SRPM URL: http://pobox.com/~tmz/fedora/python-gpod-0.4.0-3.fc7.src.rpm The packages from Comment #13 seem to work fine on x86_64 now... so I think your fix is correct. Everything looks good now from what I can see... so this package is APPROVED. You can continue the process from: http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907 I'd be happy to sponsor you. Please let me know if I can answer any questions or assist with the process any from here. Thank you Kevin, for the review and sponsorship. I'll move on to the next step in the contributors guide and request membership in the cvsextras group. |