[Bug 1931] Review request: audacious-plugins-nonfree - Audacious media player plugins with non free dependencies

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=1931





--- Comment #2 from Hans de Goede <j.w.r.degoede at gmail.com>  2011-09-24 20:27:41 ---
Hi,

Thanks for the review!

I've some questions / remarks to your remarks, see below.

(In reply to comment #1)
> > audacious-plugins-3.0.2-hardsid.patch
> 
> Doubtful patch. src/sid/xmms-sid.h says:
> 
> | /* HardSID-support is not working and is untested, thus we disable it here.
> |  */
> | #undef HAVE_HARDSID_BUILDER
> 
> This overrides config.h settings, and in turn, conditional code for HardSID
> support is not compiled. Linking the harsid-builder lib is useless.
> 

Ok, still once this get fixed, the configure check needs to be fixed too. But
indeed useless for the packages atm then, I'll send this upstream instead.

> Further, the plugin's GUI pieces are #if'ed out because they have not been
> ported to Audacious Preferences Widgets. This implies one cannot reconfigure
> the plugin with the GUI, but only by editing Audacious' config file.
> 
> Apart from that, HardSID has been (or still is) a niche market soundcard that
> can be populated with real SID chips. I can't resolve http://www.hardsid.com
> currently. It could be down or gone. I don't know anything about the more
> recent devices from that project, but the early ones have had timing issues
> with critics claiming that they have had varying results compared with either
> emulation or The Original sids.
> 

I know what the HardSID is, it just seemed wrong to me that the configure
check failed even though I did have a libsidplay 2 with HardSID support...

> [...]
> 
> Only slightly related, refer to 
> http://lists.rpmfusion.org/pipermail/rpmfusion-developers/2011-September/010163.html
>  for a comment on libsidplayfp, which I've been pointed at. It could be that
> some projects will switch to it.
> 
> [...]
> 
> You could trim down the %description as it isn't necessary to describe
> Audacious here, and that description is out-of-date anyway.
> 

Will do.

> [...]
> 
> >        --disable-sse2 \
> 
> This option has been dropped.
> 

Will drop.

> [...]
> 
> You could conflict explicitly with the "audacious-plugins-sid" package.
> 

Ah, you put the sid plugin build with just libsidplay 1 in its own sub-package,
I was under the mistaken impression you had dropped it completely, my bad.

Since c offers a "better" audacious-plugins-sid, wouldn't it be better to
obsolete it instead, I really like to avoid conflicts
where ever possible. The question then becomes Obsolete which version I guess
anything < %{version} of audacious-plugins-nonfree?


> [...]
> 
> Have built this successfully (x86_64), installed it, and it works for playing a
> couple of test files in PSID v2 format. Don't have quick access to any PSID v3
> files.
> 
> Dropping the hardsid patch and related spec sections is trivial and doesn't
> need a re-review.
> 
> APPROVED
> 

Thanks!


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.