Bug 215659

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.

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.

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.

(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.

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.

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/

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
  $ 

Jochen, ping?

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.

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 :-)

(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

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.

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> - 0.5-2
- Drop "A " from the summary
- Use "install -m 0644" instead of "cp".

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.

Sure, I'll take over. Expect (hopefully) a first review later today.

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.

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.

Looks like 0.7 will be OK license-wise once it's been released:

http://jerakeen.org/blog/2007/03/more-on-daap-licensing/

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.

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 ;-)

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.

New Package CVS Request
=======================
Package Name: python-daap
Short Description: DAAP client implemented in Python
Owners: jeff
Branches: devel FC-6
InitialCC: