Bug 474908 - Review Request: xmms2 - A modular audio framework and plugin architecture
Review Request: xmms2 - A modular audio framework and plugin architecture
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Orcan Ogetbil
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 474909 474910
  Show dependency treegraph
 
Reported: 2008-12-05 16:22 EST by Tom "spot" Callaway
Modified: 2008-12-13 10:05 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-13 09:54:36 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
oget.fedora: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Tom "spot" Callaway 2008-12-05 16:22:45 EST
Spec URL: http://www.auroralinux.org/people/spot/review/new/xmms2.spec
SRPM URL: http://www.auroralinux.org/people/spot/review/new/xmms2-0.5-1.fc11.src.rpm
Description: 
XMMS2 is an audio framework, but it is not a general multimedia player - it
will not play videos. It has a modular framework and plugin architecture for
audio processing, visualisation and output, but this framework has not been
designed to support video. Also the client-server design of XMMS2 (and the
daemon being independent of any graphics output) practically prevents direct
video output being implemented. It has support for a wide range of audio
formats, which is expandable via plugins. It includes a basic CLI interface
to the XMMS2 framework, but most users will want to install a graphical XMMS2
client (such as gxmms2 or esperanza).
Comment 1 Orcan Ogetbil 2008-12-06 15:42:28 EST
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)
Comment 2 Tom "spot" Callaway 2008-12-10 15:35:55 EST
(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@debian.org>
        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
Comment 3 Orcan Ogetbil 2008-12-10 17:31:30 EST
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
----------------------------------------
Comment 4 Tom "spot" Callaway 2008-12-10 17:55:45 EST
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.
Comment 5 Fedora Update System 2008-12-11 08:03:39 EST
xmms2-0.5-2.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/xmms2-0.5-2.fc9
Comment 6 Fedora Update System 2008-12-11 08:03:43 EST
xmms2-0.5-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xmms2-0.5-2.fc10
Comment 7 Fedora Update System 2008-12-13 09:54:33 EST
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.
Comment 8 Fedora Update System 2008-12-13 10:05:01 EST
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.

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