Bug 217066 - (python-gpod-review) Review Request: python-gpod - A python module to access iPod content
Review Request: python-gpod - A python module to access iPod content
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 219059
  Show dependency treegraph
 
Reported: 2006-11-23 11:35 EST by Todd Zullinger
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-12-04 20:44:04 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 Todd Zullinger 2006-11-23 11:35:29 EST
Spec URL: http://pobox.com/~tmz/fedora/python-gpod.spec
SRPM URL: http://pobox.com/~tmz/fedora/python-gpod-0.4.0-1.fc6.src.rpm
Description: A python module to access iPod content.  This module provides bindings to the libgpod library.

This is my first package for FE, though I've been building rpms for a while now.

Please see BZ 211648 for discussion of why this package is necessary.  Note that it depends on libgpod-0.4.0 from development.
Comment 1 Todd Zullinger 2006-11-24 03:27:32 EST
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
Comment 2 Jef Spaleta 2006-11-24 23:33:52 EST
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
Comment 3 Jef Spaleta 2006-11-24 23:42:41 EST
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
Comment 4 Todd Zullinger 2006-11-25 00:23:27 EST
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.
Comment 5 Todd Zullinger 2006-11-25 00:27:19 EST
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).
Comment 6 Brian Pepple 2006-11-28 15:09:40 EST
Todd, with the upcoming merging of Core & Extras does it make sense to create a
separate package for the python bindings?
Comment 7 Todd Zullinger 2006-11-28 15:25:46 EST
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.
Comment 8 Todd Zullinger 2006-11-28 19:12:15 EST
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.
Comment 9 Kevin Fenzi 2006-12-02 22:47:48 EST
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...
Comment 10 Todd Zullinger 2006-12-02 23:09:58 EST
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.)
Comment 11 Kevin Fenzi 2006-12-02 23:47:32 EST
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. 
Comment 12 Todd Zullinger 2006-12-03 01:13:12 EST
A key is on its way.  Thanks for the offer and the testing.
Comment 13 Todd Zullinger 2006-12-03 04:01:35 EST
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
Comment 14 Kevin Fenzi 2006-12-03 12:45:20 EST
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. 
Comment 15 Todd Zullinger 2006-12-03 21:15:35 EST
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.

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