Bug 474908

OK, I finished the review. There are a few minor issues (*) and I also have some questions (?):

- rpmlint says
     xmms2-devel.x86_64: W: no-documentation
     xmms2-perl.x86_64: W: no-documentation
     xmms2-python.x86_64: W: no-documentation
     xmms2-ruby.x86_64: W: no-documentation
but I couldn't find anything relevant in the available docs to put into these packages (except maybe the license files?), so these warnings can be ignored.

? I didn't find any occurrences of the clauses "GPL" or "Artistic" or "licensed under the same terms as perl itself." among the perl files. Is there a rule that any perl program has to be released under "GPL+ or Artistic"? I would also like to remind you that there is a .so file in the perl package that links against the libraries in the main package.

- TODO file can be included in %doc

? Is the doxygen documentation useless?

? What package(s) own the
     %{perl_archlib}/Audio/
     %{perl_archlib}/auto/Audio/
directories? Are they among the dependencies?

? Some files in the wafadmin directory have /usr/local/ /usr/lib/ directories hardcoded. Will these have any effect on the application?

? There are no .desktop files but why are there pixmaps?

* The devel package has .pc files, hence we must require pkgconfig.

* Please add the -v flag to the waf script so we can see what it is actually doing. When I did this, I found that the fedora specific compiler flag -O2 is being overwritten. This needs fixed.

? The following provides seemed weird to me:
     $ rpm -qp --provides xmms2-devel-0.5-1.fc10.x86_64.rpm
     (git
     DrLecter
     b63ec5a270cfde0ae3d59c9b89d860b8650e430f-dirty)
     commit:
     pkgconfig(xmms2-client) = 0.5
     pkgconfig(xmms2-client-cpp) = 0.5
     pkgconfig(xmms2-client-cpp-glib) = 0.5
     pkgconfig(xmms2-client-ecore) = 0.5
     pkgconfig(xmms2-client-glib) = 0.5
     pkgconfig(xmms2-plugin) = 0.5
     xmms2-devel = 0.5-1.fc10
     xmms2-devel(x86-64) = 0.5-1.fc10
What is that git parenthesis about? Is that normal?

* The devel package must require glib2-devel, qt-devel, boost-devel at the least (but I think this is all). 

* Double BR: libmodplug-devel

* Unnecessary BRs: libcurl-devel (picked up by ecore-devel), glib2-devel (picked up by avahi-glib-devel, pulseaudio-libs-devel...), libogg-devel, libvorbis-devel (both picked up by libshout-devel), python-devel (picked up by Pyrex)

(In reply to comment #1)

> ? I didn't find any occurrences of the clauses "GPL" or "Artistic" or "licensed
> under the same terms as perl itself." among the perl files. Is there a rule
> that any perl program has to be released under "GPL+ or Artistic"? I would also
> like to remind you that there is a .so file in the perl package that links
> against the libraries in the main package.

From COPYING:
src/clients/lib/perl/:
        Copyright (C) 2006-2007 Florian Ragwitz <rafl>
        Licensed under the same terms as Perl itself.

License attribution also shows up in the .xs files in src/clients/lib/perl/
 
> - TODO file can be included in %doc

Good point.

> ? Is the doxygen documentation useless?

Nope. I've added a -docs subpackage for all 6.4 MB of it.

> ? What package(s) own the
>      %{perl_archlib}/Audio/
>      %{perl_archlib}/auto/Audio/
> directories? Are they among the dependencies?

Multiple ownership for perl directories is acceptable. See:
http://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership

Since nothing in this depends on any other perl module which may own %{perl_archlib}/Audio/ or %{perl_archlib}/auto/Audio/, it is acceptable for this package to own it.

> ? Some files in the wafadmin directory have /usr/local/ /usr/lib/ directories
> hardcoded. Will these have any effect on the application?

Not in my testing on x86_64, no. The /usr/local instances are being overridden, but it can't hurt to sed replace libdir, so I've done that.

> ? There are no .desktop files but why are there pixmaps?

They're provided in case third party clients want to use "approved" images.

> * The devel package has .pc files, hence we must require pkgconfig.

Fixed.

> * Please add the -v flag to the waf script so we can see what it is actually
> doing. When I did this, I found that the fedora specific compiler flag -O2 is
> being overwritten. This needs fixed.

Good point. Added -v to build, added a patch to disable the extra -O0 that was being appended to CFLAGS/CPPFLAGS.

> ? The following provides seemed weird to me:
>      $ rpm -qp --provides xmms2-devel-0.5-1.fc10.x86_64.rpm
>      (git
>      DrLecter
>      b63ec5a270cfde0ae3d59c9b89d860b8650e430f-dirty)
>      commit:
>      pkgconfig(xmms2-client) = 0.5
>      pkgconfig(xmms2-client-cpp) = 0.5
>      pkgconfig(xmms2-client-cpp-glib) = 0.5
>      pkgconfig(xmms2-client-ecore) = 0.5
>      pkgconfig(xmms2-client-glib) = 0.5
>      pkgconfig(xmms2-plugin) = 0.5
>      xmms2-devel = 0.5-1.fc10
>      xmms2-devel(x86-64) = 0.5-1.fc10
> What is that git parenthesis about? Is that normal?

Hmm. It looks like it is getting implanted into one of the .pc files, then rpm is scraping it out as a Provides for some reason. Easy enough to fix the wscript to have a little versioning sanity.

> * The devel package must require glib2-devel, qt-devel, boost-devel at the
> least (but I think this is all). 

Added.

> * Double BR: libmodplug-devel

Fixed.

> * Unnecessary BRs: libcurl-devel (picked up by ecore-devel), glib2-devel
> (picked up by avahi-glib-devel, pulseaudio-libs-devel...), libogg-devel,
> libvorbis-devel (both picked up by libshout-devel), python-devel (picked up by
> Pyrex)

Fixed.

Okay, here is the new SPRM and SPEC:

New SRPM: http://www.auroralinux.org/people/spot/review/new/xmms2-0.5-2.fc11.src.rpm

New SPEC: http://www.auroralinux.org/people/spot/review/new/xmms2.spec

Thanks for the update. The package is good to go now.

Btw, the following issues may need to be re-addressed by your friend for the freeworld package ;)

> ? Some files in the wafadmin directory have /usr/local/ /usr/lib/ directories
> hardcoded. Will these have any effect on the application?

> * Please add the -v flag to the waf script so we can see what it is actually
> doing. When I did this, I found that the fedora specific compiler flag -O2 is
> being overwritten. This needs fixed.
 
> * Double BR: libmodplug-devel
 
> * Unnecessary BRs: libcurl-devel (picked up by ecore-devel), glib2-devel
> (picked up by avahi-glib-devel, pulseaudio-libs-devel...), libogg-devel,
> libvorbis-devel (both picked up by libshout-devel), python-devel (picked up by
> Pyrex)


----------------------------------------
This package (xmms2) is APPROVED by oget
----------------------------------------

New Package CVS Request
=======================
Package Name: xmms2
Short Description: A modular audio framework and plugin architecture
Owners: spot
Branches: F-9 F-10 devel
InitialCC:

... and it's done.

xmms2-0.5-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/xmms2-0.5-2.fc9

xmms2-0.5-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xmms2-0.5-2.fc10

xmms2-0.5-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

xmms2-0.5-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.