Bug 215659 - Review Request: python-daap - A DAAP client implemented in Python
Review Request: python-daap - A DAAP client implemented in Python
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Saou
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-11-14 21:03 EST by Jeffrey C. Ollie
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-03-17 12:54:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
matthias: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeffrey C. Ollie 2006-11-14 21:03:12 EST
Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.4-1.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.4-1.fc6.src.rpm
Description:

A DAAP client implemented in Python.
Comment 1 Jochen Schmitt 2006-11-16 14:00:10 EST
Good:
+ Rpmlint doesn't complaints the source package.
+ Local build works fine.
+ Rpmlint doesn't complaints the binary package.
+ Tar Ball matches with upstream.
+ Local Install/Uninstall works fine.

Bad:
- Strange License tag which refer to a non existing License file.
Comment 2 Jeffrey C. Ollie 2006-11-16 18:12:50 EST
Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.4-2.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.4-2.fc6.src.rpm

I've added a copy of the license text and clarified the comment regarding the
license.
Comment 3 Jochen Schmitt 2006-11-19 15:52:41 EST
Good:
+ License ok, package contains verbatin copy of the license text.
+ Naming confirm with name convention.

On the project page was a posting about a severe problem. It will be
nice to give me a method to test the library, becouse it seems to be
very unstable.
Comment 4 Jeffrey C. Ollie 2006-11-19 21:15:46 EST
(In reply to comment #3)
>
> On the project page was a posting about a severe problem. It will be
> nice to give me a method to test the library, becouse it seems to be
> very unstable.

I think that the comment that you refer to was posted before the 0.4 version was
released.
Comment 5 Jochen Schmitt 2006-11-20 10:55:51 EST
That my be, but unfortunately I can't get this itshell programm talk on the
homepage of the project. do you know how I can get it to test your package.
Comment 6 Jeffrey C. Ollie 2006-11-20 12:34:41 EST
I know that the latest versions of Elisa use PythonDaap, unfortunately Elisa has
a number of dependencies that aren't in Fedora yet.  The main one is
python-twisted-core - it's been reviewed and accepted but it hasn't been built
and pushed out yet.

http://www.fluendo.com/elisa/
Comment 7 Jeffrey C. Ollie 2006-11-20 13:10:47 EST
I was able to get itshell.py working, it's a little non-intuitive...  First, you
need to set up a DAAP server.  I used Rhythmbox running on another PC, but
iTunes should work... There are probably other music players out there that can
be DAAP servers...

  $ python itshell.py 
  
      The python/daap interactive shell.
      Type 'help' for help.
      
  (no server): connect 10.0.0.1
  Connecting to '10.0.0.1':3689
  2006-11-20 12:08:30,358 DEBUG getting /content-codes
  2006-11-20 12:08:30,408 DEBUG getting /server-info
  2006-11-20 12:08:30,411 DEBUG getting /login
  2006-11-20 12:08:30,412 DEBUG Logged in as session -1546218061
  2006-11-20 12:08:30,413 DEBUG getting /databases?session-id=-1546218061
  2006-11-20 12:08:30,415 DEBUG getting /databases?session-id=-1546218061
  using database 'u"Jeff's Music"'
  2006-11-20 12:08:30,416 DEBUG getting
/databases/1/items?session-id=-1546218061&meta=dmap.itemid,dmap.itemname,daap.songalbum,daap.songartist,daap.songformat,daap.songtime
  Got 52 tracks
  (10.0.0.1:3689): search elvis
  1 tracks found.
  45: u'Elvis Presley' - u'The Time-Life Treasury of Christmas (disc 1)' -
u'Here Comes Santa Claus'
  (10.0.0.1:3689): download 45 claus.flac
  2006-11-20 12:09:09,385 DEBUG saving to 'claus.flac'
  2006-11-20 12:09:09,386 DEBUG getting
/databases/1/items/45.flac?session-id=-1546218061
  2006-11-20 12:09:10,422 DEBUG Done
  (10.0.0.1:3689): exit
  bye.
  2006-11-20 12:09:27,502 DEBUG getting /logout?session-id=-1546218061
  2006-11-20 12:09:27,504 DEBUG DAAPSession: expired session id -1546218061
  $ 
Comment 8 Jeffrey C. Ollie 2007-02-12 12:46:57 EST
Jochen, ping?
Comment 9 Matthias Saou 2007-02-12 12:59:32 EST
Just looked at the last spec file and it looks good to me, apart from the
comments that seems to be leftover from a template, thus could be removed.

Regarding Elisa, it requires a more recent version of PythonDAAP now. For the
package I had made, I had created this patch to bring 0.4 up to what's in SVN :

http://ftp.es6.freshrpms.net/tmp/extras/PythonDaap-0.4-r3266.patch

Note that it also contains the LICENSE file you've already added too ;-)
If Jochen doesn't finish reviewing this package, I wouldn't mind doing it.
Comment 10 Matthias Saou 2007-02-12 13:33:18 EST
I just asked the author if he'd be willing to make a new release ASAP in order
to not have to package an svn snapshot... and he did! So 0.5 is now available,
and will probably even include the license file :-)
Comment 11 Jeffrey C. Ollie 2007-02-12 14:18:17 EST
(In reply to comment #10)
> I just asked the author if he'd be willing to make a new release ASAP in order
> to not have to package an svn snapshot... and he did! So 0.5 is now available,

Yes, I saw that...

> and will probably even include the license file :-)

Unfortunately, he didn't.

Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.5-1.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.5-1.fc6.src.rpm
Comment 12 Matthias Saou 2007-02-12 14:24:40 EST
Too bad for the license, then...

Anyway, two more quickies :
- The consensus would seem to be to remove the leading "A " from the summary
- The "cp" for SOURCE1 is wrong, as a bad umask can lead to wrong file modes,
which is known to have happened in the build system, use "install -m 0644" instead

The rest looks quite alright to me. Jochen : Please review ASAP or let me know
if you want me to pick this one up.
Comment 13 Jeffrey C. Ollie 2007-02-12 14:36:04 EST
Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.5-2.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.5-2.fc6.src.rpm

%changelog
* Mon Feb 12 2007 Jeffrey C. Ollie <jeff@ocjtech.us> - 0.5-2
- Drop "A " from the summary
- Use "install -m 0644" instead of "cp".
Comment 14 Jochen Schmitt 2007-02-15 14:28:27 EST
Hello,

unfortunately, I'm buissy with some multilib issues, so I don't have any time
for this review now. It will be nice, if anyone can over my part.
Comment 15 Matthias Saou 2007-02-16 05:03:16 EST
Sure, I'll take over. Expect (hopefully) a first review later today.
Comment 16 Matthias Saou 2007-02-19 05:34:45 EST
rpmlint output on the source rpm :
W: python-daap mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 15)
rpmlint output on the binary rpm is ok.

The spec file and resulting packages seem to comply with all MUST review items.

The only part which is quite vague is the LICENSE. The author has made a
statement about relicensing under the LGPL, but :
1) The source tarball for 0.5 still doesn't contain the LICENSE file
2) The PKG-INFO file still contains "License: UNKNOWN"
3) The sources contain two files (md5c.c and md5.h) which were taken from the
Python sources and have their own "RSA Data Security, Inc." license.

Points 1) and 2) could be easily dealt with by sending a quick patch to the
author. The most surprizing part is that the LICENSE file is actually in the svn
trunk, and has been for a while, but was skipped when making the 0.5 release it
seems.
Point 3) is a bit trickier, as I have no idea if that license is actually LGPL
compatible or not (I would think it is, though). If it isn't, then we have a
problem. If it is, then simply adding a note about "Parts of the software are
derived from the RSA Data Security, Inc. MD5 Message-Digest Algorithm" +
possibly a copy of the RSA terms should be enough.
Comment 17 Matthias Saou 2007-03-02 06:38:34 EST
I just came across an interesting thread here :
http://mail.python.org/pipermail/python-dev/2005-February/051450.html

Looks like the current content of the source might not be acceptable and might
need to be modified. Some possibilities are discussed further in the thread.
Comment 18 Jeffrey C. Ollie 2007-03-12 15:34:22 EDT
Looks like 0.7 will be OK license-wise once it's been released:

http://jerakeen.org/blog/2007/03/more-on-daap-licensing/
Comment 19 Jeffrey C. Ollie 2007-03-12 23:34:55 EDT
Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.7-1.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.7-1.fc6.src.rpm

OK, I think that I have all the issues fixed in this one.  The LICENSE file is
included in the upstream tarball and the problematic MD5 code has been replaced
with a public domain version.
Comment 20 Matthias Saou 2007-03-13 06:07:25 EDT
Great! Tom Insam the author) has been really receptive to the licensing
concerns, which should indeed be fixed now. Now that things are clear, you could
remove the "See http://jerakeen.org/code/pythondaap/#comment_643" lines above
the "License:".

Nitpicks :
- Remove the comments above the License line of the spec.
- Add a %changelog entry for 0.7-1.
- I'd put "examples/" instead of "examples" for %doc to show it's a directory.
- You might want to include the PKG-INFO in %doc now that it's correct.

Formal review :
This is a quite trivial python package, which contains all what a python package
should. It's not noarch because of the included md5 module, the LGPL license is
correct and rpmlint is silent on the resulting packages... APPROVED. Feel free
to "fix" any of the nitpicks from above, though ;-)
Comment 21 Jeffrey C. Ollie 2007-03-13 11:52:42 EDT
Spec URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.7-2.fc6.spec
SRPM URL: http://repo.ocjtech.us/misc/fedora/6/SRPMS/python-daap-0.7-2.fc6.src.rpm

For reference, here are the spec & SRPM with the recommended changes.
I had made some of them before but I must have forgotten to upload the
correct files.
Comment 22 Jeffrey C. Ollie 2007-03-13 11:54:55 EDT
New Package CVS Request
=======================
Package Name: python-daap
Short Description: DAAP client implemented in Python
Owners: jeff@ocjtech.us
Branches: devel FC-6
InitialCC: 

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