Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 215165 - Review Request: audacious-plugins - Plugins for the Audacious media player
Summary: Review Request: audacious-plugins - Plugins for the Audacious media player
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Parag AN(पराग)
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 214818
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-11-11 19:29 UTC by Ralf Ertzinger
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-26 20:12:21 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Ralf Ertzinger 2006-11-11 19:29:19 UTC
Spec URL: http://www.skytale.net/files/audacious/audacious-plugins.spec
SRPM URL: http://www.skytale.net/files/audacious/audacious-plugins-1.2.2-0.5.sky.src.rpm
Description:

Audacious is a media player that currently uses a skinned
user interface based on Winamp 2.x skins. It is based on ("forked off")
BMP.
This package provides essential plugins for audio input, audio output
and visualization.

Comment 1 Ralf Ertzinger 2006-11-14 20:33:16 UTC
New version to fix missing build requires at http://www.skytale.net/files/audacious/

Needs a newer version of audacious as well, available at the same place.

Comment 2 Parag AN(पराग) 2006-11-16 04:43:26 UTC
I dont think you need Obsoletes tag for subpackages -arts, -jack, -esd. Because
they are throwing a rpmlint error.

Comment 3 Ralf Ertzinger 2006-11-16 08:42:53 UTC
The obsoletes are necessary, because the old audacious package provided these
extra packages, too, and they need to be replaced on upgrade.

Do you refer to the message that the subpackages have Obsoletes:, but no Provides:?

Comment 4 Parag AN(पराग) 2006-11-16 09:07:40 UTC
(In reply to comment #3)
> The obsoletes are necessary, because the old audacious package provided these
> extra packages, too, and they need to be replaced on upgrade.
ok
> 
> Do you refer to the message that the subpackages have Obsoletes:, but no
Provides:?

Yes.

Comment 5 Ralf Ertzinger 2006-11-16 09:26:25 UTC
I am not sure about this one.

For one, there ought to be no packages which Require: audacious-arts, for
example, so there is no gain in the new packages Providing: it.

In addition, if there were any other packages which actually Require: one of the
Obsoleted: packages, they probably need to be rebuilt to match the new codebase,
anyway, so there ought to be a RPM conflict.

So, in conclusion, I think that this specific rpmlint warning can be ignored.
I'd be happy about a third opinion, though.

Comment 6 Dominik 'Rathann' Mierzejewski 2006-11-17 02:41:38 UTC
(In reply to comment #5)
> So, in conclusion, I think that this specific rpmlint warning can be ignored.
> I'd be happy about a third opinion, though.

I think it can be ignored, too.

I have another comment, however. I know it's a matter of preference, but why not
put each BuildRequired package in its own line and sort the list alphabetically?
It makes it easier to spot missing builddeps, for example: fluidsynth-devel and
libglade2-devel. The package doesn't build here without the latter.

Comment 7 Matthias Saou 2006-11-17 10:20:11 UTC
Yeah, this also seems correct to me : No need to "provide" the old names, as
they shouldn't have been used other than explicitly (not from other package
requirements). The obsoletes are required though, to provide a clean upgrade
path, and are correct with the last known version.

Please start the review ASAP or let me know if you'd like me to do it instead,
as I'm quite impatient to have this audacious update available ;-)

Comment 8 Parag AN(पराग) 2006-11-17 11:09:44 UTC
Ok. Based on you comment i am going to review this package now.

Comment 9 Parag AN(पराग) 2006-11-17 11:41:16 UTC
(In reply to comment #7)
> Yeah, this also seems correct to me : No need to "provide" the old names, as
> they shouldn't have been used other than explicitly (not from other package
> requirements). The obsoletes are required though, to provide a clean upgrade
> path, and are correct with the last known version.
> 
> Please start the review ASAP or let me know if you'd like me to do it instead,
> as I'm quite impatient to have this audacious update available ;-)

Thanks for testing this package.
Based on above comment
Review
+ package builds in mock (development i386)FC7.
+ rpmlint is silent for SRPM.
+ rpmlint on RPMs is not silent but as per above comments in bugzilla they
  can be ignored.
+ source files match upstream.
8ac7f73da7432e1ffed6c2b9b0fced8c  audacious-plugins-fedora-1.2.2.tar.gz
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.  License text included in package.
+ %doc is small; no -doc subpackage required.
+ %doc does not affect runtime.
+ COPYING included in %doc.
+ BuildRequires are proper.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code, not content.
+ no headers or static libraries.
+ no .pc files.
+ no -devel subpackage exists
+ available subpackages are audacious-plugins-jack,audacious-plugins-arts,
   audacious-plugins-esd,audacious-plugins-pulseaudio
+ as subpackages are packaging .so files post and postun called /sbin/ldconfig
+ Used update-desktop-database correctly
+ no .la files.
+ no translations available
- Does NOT owns the directories it creates.
+ no duplicates in %files.
+ file permissions are appropriate.

SHOULD:-
  I saw that some directories are owned in audacious rpm whereas there is no
need to own them by audacious but it should be owned by audacious-plugins.
like
/usr/lib/audacious
/usr/lib/audacious/Container
/usr/lib/audacious/Effect
/usr/lib/audacious/General
/usr/lib/audacious/Input
/usr/lib/audacious/Output
/usr/lib/audacious/Visualization
Make it own by audacious-plugins

Comment 10 Parag AN(पराग) 2006-11-17 11:56:41 UTC
Matthias Saou,
      What do you think is above question is valid or should i approve package
as it is?

Comment 11 Ralf Ertzinger 2006-11-17 12:07:22 UTC
OK, one might argue that a-p should own /usr/lib/audacious, since audacious
itself does not place any files there.

If (and that's a big if) we/I should ever decide to split up a-p into a lot of
tiny subpackages these directories would have to go back to audacious, since no
subpackage could own all of these dirs.

I personally do not care too much about it, but given the choice would leave
things as they are (reduces work for me :)

Comment 12 Matthias Saou 2006-11-17 12:25:08 UTC
Since we have audacious requiring audacious-plugins, and audacious-plugins
requiring audacious, I'm not quite sure what the proper solution would be. A
while back, I would have made those dirs owned by both packages, in order to be
100% sure they get rmdir'ed upon removal of both packages (since we don't know
which will be removed last in the same rpm transaction), but I'm not sure this
is compatible with today's packaging guidelines.
You might want to ask on the packaging list or go through the guidelines again ;-)

Comment 13 Ralf Ertzinger 2006-11-17 12:31:12 UTC
Erm, audacious Requires: audacious-plugins.
audacious-plugins Requires: libaudacious.so.4 (provided by audacious-libs)
audacious-plugins does not require audacious (circular deps, ugly).

So there is nothing that prevents audacious from being removed while a-p is
still installed as far as I can see.

Ugly, that.
Make audacious-libs own the plugin-dirs?

Comment 14 Matthias Saou 2006-11-17 13:16:22 UTC
I just looked again at audacious.spec :

Requires:       audacious-plugins >= 1.2.0

And at audacious-plugins.spec :

Requires:       audacious >= 1.2.0

But maybe those two spec files aren't the latest?

Comment 15 Ralf Ertzinger 2006-11-17 14:05:25 UTC
You're right, I forgot that (splitting off audacious-libs solved the build time
deps, however).

But even the explicit dependency does not guarantee removal in the right order,
or does it?

Any bright ideas on that?

Comment 16 Parag AN(पराग) 2006-11-20 04:54:58 UTC
Ralf,
    Got any working solution?may be then you can do make a-p own audacious-libs.
Just a thought.

Comment 17 Ralf Ertzinger 2006-11-20 08:19:01 UTC
I think I will make a-p own /usr/lib/audacious. This does not solve the question
which package is to be removed first (audacious or a-p), but at least the
directory structure will be cleanly installable and removable.

Modified packages forthcoming.

Comment 18 Ralf Ertzinger 2006-11-21 09:54:57 UTC
New audacious and audacious-plugins packages at
http://www.skytale.net/files/audacious/
Moves /usr/lib/audacious into audacious-plugins, and adds a patch for cdaudio.

Comment 19 Parag AN(पराग) 2006-11-21 10:24:51 UTC
Unable to download files

Comment 20 Parag AN(पराग) 2006-11-21 12:21:51 UTC
Ok got the access.
I think packaging is OK now.
APPROVED from me but do anyone have still any objections?


Comment 21 Dominik 'Rathann' Mierzejewski 2006-11-21 13:13:57 UTC
fluidsynth alsa-midi plugin is still missing, see comment #6.

Comment 22 Ralf Ertzinger 2006-11-21 17:46:54 UTC
fluidsynth added

Comment 23 Parag AN(पराग) 2006-11-22 04:00:04 UTC
where? why are you not posting updated direct download links here?

Comment 24 Ralf Ertzinger 2006-11-22 18:42:56 UTC
It's at the same place all the other versions were, posted several times though
this review. I thought that was kind of evident.

http://www.skytale.net/files/audacious/audacious-plugins-1.2.2-0.8.sky.src.rpm
http://www.skytale.net/files/audacious/audacious-plugins.spec

Comment 25 Parag AN(पराग) 2006-11-23 04:18:39 UTC
Thanks APPROVED.

Comment 26 Ralf Ertzinger 2006-11-24 12:02:54 UTC
Thanks.


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