Some quick initial remarks:
Why are these lines there but commented, for FC-5 ? :
# This is probably temporary until the above libs get their dependencies
# right
# BuildRequires:  libSM-devel libICE-devel libX11-devel libXrandr-devel
# BuildRequires:  libXau-devel libXi-devel libXft-devel libXinerama-devel
# BuildRequires:  libXcursor-devel libXrender-devel libXext-devel libXfixes-devel
# BuildRequires:  libXdmcp-devel libXt-devel

Why doesn't this obsolete and provides the current version of bmp. This is a
newer bugfixed/enhanced version of bmp right? Is there anything bmp can do that
this can't?

I thought this was intended as a bmp (and ultimatly xmms) replacement. Or are
you planning on maintaining audacious next to bmp? In that case do we really
want 3 xmms versions (the original + 2 forks) in FE?

I'll try todo a full review tomorrow. For today I've spend enough time behind my PC.

I don't see anything wrong with the NES/Gameboy/PlayStation plugins. They
operate basically like the SID players that are already in extras. I don't see
any proprietary ROM code in them, they just emulate the sound and CPU chips. But
then IANAL. The PSX plugin does have a bunch of ROM calls thunked to native code.

(In reply to comment #2)
> then IANAL. The PSX plugin does have a bunch of ROM calls thunked to native code.


IOW, it doesn't include any ROM code either, correct?

The spec file is based on my spec file for BMP, the commented out build deps are
from that build. They are not needed anymore and will be removed.

I have thought about obsoleting bmp. I am not sure we should do this for FC6 for
the following reasons:

a) there may be additional plugins for bmp in FE that are not ported to
audacious yet, so existing users would lose playback ability. I have not checked
this yet, so it may be a non-issue.
b) obsoleting bmp with audacious leads to a continuity problem from the view of
the user. The user who has been using bmp gets (maybe unsuspectingly) a new
program which looks similar, behaves simimar, but has forgotten all his
settings. Maybe we should come up with a way to import BMP settings on the first
startup.

(In reply to comment #4)
> I have thought about obsoleting bmp. I am not sure we should do this for FC6 for
> the following reasons:
> 
> a) there may be additional plugins for bmp in FE that are not ported to
> audacious yet, so existing users would lose playback ability. I have not checked
> this yet, so it may be a non-issue.

This is a non-issue according to owners.list, the only plugin currently
available through FE is bmp-flac2. AFAIK this is included into audacious, right,
otherwise we can port it to audacious, make it obsolete bmp-flac2 and release it
at the same time.

> b) obsoleting bmp with audacious leads to a continuity problem from the view of
> the user. The user who has been using bmp gets (maybe unsuspectingly) a new
> program which looks similar, behaves simimar, but has forgotten all his
> settings. Maybe we should come up with a way to import BMP settings on the first
> startup.

Hmm,

I see that this is not entirely user friendly, but really you cannot configure
that much with bmp / audacious. Many other smaller opensource projects sometimes
change there configuration format without providing a conversion tool. Only the
real big ones provide conversion tools like firefox and ooo.

To me the extra effort of having to maintain 2 programs +plugins versus 1 muich
outweighs this disadvantage of obsoleting bmp.

Since I am the maintainer (and upstream) of bmp-flac2, this is mostly a
nonissue, right :)

I'll look into the configuration thing again. Maybe it is simple to merge in the
old bmp configuration.

Right, the PSX player traps ROM calls to native code, it does not have any
actual ROM code in it that I can see.

Also, the sooner we can get an audacious-mp3 package into the Repo That Must Not
Be Named, the better. ;P

And ACC and WMA. Those codecs were surgically removed from the tarball :)

I had promised todo a full review today, but I'm afraid that aint going to
happen. I'll try todo it tomorrow. (Today I've sunk all my spare time into
writing a driver for the second generation of Abit uGuru hw-monitoring chips,
based on reverse enginneering because Abit won't release the specs GRRR. I had
just finished my driver for the first generation when I started getting mails
that that driver didn't work on newer motherboards :|   )

Okay something weird is going on. It works fine on my i386 laptop, but on my old
PPC iMac adding files is really broken. Stuff will show up in the playlist
URL-encoded (%20's etc) and they won't play. Drag-and-drop from nautilus does
work however.

Anyone else using PPC?

As it happens I have an iBook myself. I'll try and reproduce this.

MUST:
=====
* rpmlint output is:
W: audacious strange-permission audacious-fedora-1.1.0-dr2.tar.gz 0600
W: audacious strange-permission audacious.spec 0600
W: audacious strange-permission audacious-1.1.0-xmms-skins.patch 0600
W: audacious strange-permission audacious-1.1.0-default-skin.patch 0600
W: audacious prereq-use desktop-file-utils >= 0.9
W: audacious incoherent-version-in-changelog 1.1.0-dr2 1.1.0-0.0.dr2
E: audacious binary-or-shlib-defines-rpath /usr/bin/audacious ['/usr/lib']
W: audacious symlink-should-be-relative /usr/lib/libaudacious.so.2
/usr/lib/libaudacious.so.2.0.0
W: audacious-devel no-documentation
W: audacious-devel symlink-should-be-relative /usr/lib/libaudacious.so
/usr/lib/libaudacious.so.2
These all must be fixed except for the no-doc warning for -devel
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream, expect for stripped parts
* Compiles and builds on devel-i386
* BR: some are missing see must fix
* Locales handled properly
* ldconfig properly called for shared libs
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* -devel package as needed
* no .la files
* .desktop file as required and properly installed


MUST fix:
=========
* rpmlint output, let me know if you need help with fixing the rpath usage,
  for some reason many new packages have this and I'm getting quite skilled
  in fixing these.
* Remove the unnescesarry commented libXxxxxx Requires
* Add missing BR: lirc-devel, so that lirc support gets compiled in.
* Add missing BR: libmodplug-devel, so that modplug support gets compiled in.
* Add missing BR: bison, configure checks for bison and bison is no longer
  in our default buidroot.
* Remove --disable-oss from %configure's flags. Some people may have cards
  which only work with oss drivers and it does 0 harm (no extra deps, dynamicly
  loaded, so cannot cause any bugs when not used).
* Under %files I see %{_datadir}/pixmaps/audacious.png, that is not according to
  the freedesktop.org icon standard, it should go under:
  %{_datadir}/icons/hicolor/32x32/apps
  Where 32x32 is the size of the icon, please do ls /usr/share/icons/hicolor/
  to see the available valid sizes, if the icon doesn't match any pick the 
  closest.
* Once the icon is in the proper case you must add %post(un) script to update 
  the icon-cache see:
http://fedoraproject.org/wiki/ScriptletSnippets#head-fc74f078205565f961f6d836b77c3428619c689d
* remove the spurious: # rm -f $RPM_BUILD_ROOT%{_infodir}/dir

Should fix:
===========
* I think it would be better to only strip the dirs containing the offending
  code and to not change Makefile.in and configure in the tarball, keeping
  things as much in sync with upstream impossible. Put the nescesarry
  Makefile.in and configure changes in a patch instead.
* Obsolete: bmp
* Most packages I've seen do this:
desktop-file-install --vendor fedora \
    --dir $RPM_BUILD_ROOT%{_datadir}/applications   \
    --add-category X-Fedora  \
    --add-category Application  \
    --remove-mime-type audio/mp3 \
    --remove-mime-type audio/x-mp3 \
    --remove-mime-type audio/mpeg \
    --remove-mime-type audio/x-mpeg \
    audacious/audacious.desktop
rm -f $RPM_BUILD_ROOT%{_datadir}/applications/audacious.desktop

Like this:
desktop-file-install --vendor fedora \
    --dir $RPM_BUILD_ROOT%{_datadir}/applications   \
    --add-category X-Fedora  \
    --add-category Application  \
    --remove-mime-type audio/mp3 \
    --remove-mime-type audio/x-mp3 \
    --remove-mime-type audio/mpeg \
    --remove-mime-type audio/x-mpeg \
    --delete-original \
    $RPM_BUILD_ROOT%{_datadir}/applications/audacious.desktop

Hint the difference is in the last 2 lines :) The second way is advised on the
scriptlets page I believe.


Nice to Have:
=============
* Add BR: jack-audio-connection-kit-devel, libsamplerate-devel to enable
  the jack output plugin.
* Put the jack output plugin into a subpackage so that jack and samplerate
  don't get sucked in as Requires for users who don't need jack
* Put the arts output plugin into a subpackage so that qt and arts don't
  get sucked in as dependencies for non kde users.

I'll reply to some things now, having just looked over your report.

a) what exactly makes 600 a strange permission?
b) if you know how to fix the rpath thing I'll be grateful for some pointers.
c) do we even ship oss drivers anymore in FC?
d) While configure checks for bison, it does not actually use it. The package
built successfully with the minimal build root.

Created attachment 131751 [details]
Patch fixing rpath usage

(In reply to comment #13)
> I'll reply to some things now, having just looked over your report.
> 
> a) what exactly makes 600 a strange permission?
The fact that 644 is the normal permission, with 600 if user a install the srpm
user b can't read it.

> b) if you know how to fix the rpath thing I'll be grateful for some pointers.

The attached patch fixes this?

> c) do we even ship oss drivers anymore in FC?

I don't think so but one can still buy the commercial drivers which are
unfortunatly still needed for some cards. Also some people compile there own
kernels with oss drivers, because they have better luck with some cards with
OSS. Besides that enabling it is free, it only takes a tiny amount of
diskspace, its not like it drags in a whole load of unnescesarry deps

> d) While configure checks for bison, it does not actually use it. The package

> built successfully with the minimal build root.

Ok, understood.

Concerning the stripped tarball:

I actually prefer shipping (albeit just in a SRPM) a tarball which is consistent
in itself, i.e. does not need autofoomagic on behalf of a user to build (I have
not patched configure itself, I patched configure.ac and let autoconf build a
new one). This is actually less work than maintaining a patch against configure
(at least for me). SVN makes it pretty easy to track upstream this way.

(In reply to comment #15)
> Concerning the stripped tarball:
> 
> I actually prefer shipping (albeit just in a SRPM) a tarball which is consistent
> in itself, i.e. does not need autofoomagic on behalf of a user to build (I have
> not patched configure itself, I patched configure.ac and let autoconf build a
> new one). This is actually less work than maintaining a patch against configure
> (at least for me). SVN makes it pretty easy to track upstream this way.

As you wish, I thought you might have your own preferences for this and there
are no guidelines, hence it was on the should fix list.

I have uploaded new versions to http://www.skytale.net/files/audacious/, which
should fix all the issues you presented.

I have not tested ppc so far.

This is incorrect:
Requires(pre):  desktop-file-utils >= 0.9

That should be:
BuildRequires:   desktop-file-utils >= 0.9
Requires(post):  desktop-file-utils >= 0.9, /sbin/ldconfig
Requires(postun): desktop-file-utils >= 0.9, /sbin/ldconfig

Except for that its fine.

So, the (I hope) final version is up at http://www.skytale.net/files/audacious/

Looks good, approved!

Closing as this has been imported and build.

Thanks for the review.