Bug 222648

Summary: Review Request: audacious-plugin-fc - Future Composer plugin for Audacious
Product: [Fedora] Fedora Reporter: Michael Schwendt <bugs.michael>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED ERRATA QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: opensource
Target Milestone: ---Flags: j: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.2-1 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-07-05 19:24:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Michael Schwendt 2007-01-15 14:46:22 UTC
Spec URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc.spec
SRPM URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc-0.1-1.src.rpm
Description: This is an input plugin for Audacious which can
play back Future Composer music files from AMIGA. Song-length
detection and seek are implemented, too.

Note: May take a while till source tarball is available at SF.net
everywhere. As I'm upstream for the old source code, here's a detached
sig: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc-0.1.tar.bz2.asc

Input files for testing:
http://xmms-fc.sourceforge.net/favfcmods2.tgz

Comment 1 Martin Sourada 2007-01-17 21:57:14 UTC
====REVIEW CHECKLIST====
* rpmlint not silent:
  E: audacious-plugin-fc no-changelogname-tag
  E: audacious-plugin-fc unknown-key GPG#b8af1c54
- package naming guidelines met
- spec file name matches %%{name}
* packaging guidelines not met:
  - missing %%changelog entries in spec file
  - (slightly different build root from the preferred one)
- Package licensed with GPL compatible license
- %%doc section OK
- spec written in American English
- spec file legible (only you should remove in the %%install section "mkdir -p
$RPM_BUILD_ROOT" - it's not necessary; and as mentioned above, %%changelog missing)
- source match upstream (613c456b525d0f5ebce912a981e57264 
audacious-plugin-fc-0.1.tar.bz2)
- builds successfully on at least i386 FC6
- all build dependences listed
- locales handled properly (none present)
- no shared libraries
- package is not relocatable
- package owns all its directories
- dirs not owned is owned by required package(s)
- no duplicates
- file permissions set properly
- contains proper %%clean section
- macros used consistently
- package contains code
- no large docs
- no -devel subpackage
- no pkgconfig files
- no libraries with suffix, no static libraries
- not a GUI app
- package does not own dirs or files owned by other packages
- builds in mock

====Things need to be done====
Add %%changelog section to your specfile:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-b7d622f4bb245300199c6a33128acce5fb453213

You should change build root to the prefered one:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1

As mentioned above, you should remove from the %%install section "mkdir -p
$RPM_BUILD_ROOT". It is not necessary.


I am not sponsored. This pre-review is to help me get sponsored.

Comment 2 Till Maas 2007-01-17 23:03:55 UTC
Change
Source:	http://download.sourceforge.net/xmms-fc/audacious-plugin-fc-0.1.tar.bz2
to
Source:
http://download.sourceforge.net/xmms-fc/audacious-plugin-fc-%{version}.tar.bz2
So you do not need to change the Source with every new version.

Consider changing the release-tag to:
Release:        1%{?dist}

See: http://fedoraproject.org/wiki/Packaging/DistTag

And I noticed, that the other audacious plugins in extras have audacious-plugins
(plural) as prefix not only audacious-plugin (singular), e.g.
audacious-plugins-arts.

Comment 3 Michael Schwendt 2007-01-17 23:32:49 UTC
All valid points.

* changed "Buildroot"
* changed "Source"
* added %changelog for this revision
* changed %install

Spec URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc.spec
SRPM URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc-0.1-2.src.rpm

* "audacious-plugins" is a collection of multiple plug-ins, where
the sub-package names are inherited from the main package. Instead,
the packager could have named the sub-packages "audacious-plugin-foo"
explicitly. This package contains a single plugin, and its upstream
name is audacious-plugin-fc, too.

* %{?dist} can be added in CVS.

Comment 4 Michael Schwendt 2007-03-28 09:35:19 UTC
An update only when Audacious 1.3 in FE devel cvs is releaased:

Spec URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc.spec
SRPM URL: http://home.arcor.de/ms2002sep/tmp/audacious-plugin-fc-0.2-1.src.rpm

Comment 5 Jason Tibbitts 2007-06-29 01:10:47 UTC
Well, 1.3 has been in rawhide for ages now, so....

Comment 6 Jason Tibbitts 2007-06-29 02:11:16 UTC
You shouldn't need a build dependency on pkgconfig; audacious-devel should have
its own dependency (and indeed it does).  But of course it doesn't hurt anything.

It seems there's directory ownership problem; /usr/lib/audacious/Input is owned
by audacious-plugins, but they're not required by this package.  This seems like
the kind of thing the base audacious package should own.

Review:
* source files match upstream:
   deafc2c95dc7a1a67a83b53b7871686fa280c8ed18547df51988b648dbe5519b  
   audacious-plugin-fc-0.2.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   libfc.so()(64bit)
   audacious-plugin-fc = 0.2-1
  =
   audacious >= 1.3
   libatk-1.0.so.0()(64bit)
   libaudacious.so.5()(64bit)
   libcairo.so.2()(64bit)
   libgcc_s.so.1()(64bit)
   libgdk-x11-2.0.so.0()(64bit)
   libgdk_pixbuf-2.0.so.0()(64bit)
   libglib-2.0.so.0()(64bit)
   libgmodule-2.0.so.0()(64bit)
   libgobject-2.0.so.0()(64bit)
   libgtk-x11-2.0.so.0()(64bit)
   libmcs.so.1()(64bit)
   libpango-1.0.so.0()(64bit)
   libpangocairo-1.0.so.0()(64bit)
   libpng12.so.0()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)
* %check is not present; no upstream test suite.  I've no ability to actually 
   test this as I have no speakers on this machine.
* no shared libraries are added to the regular linker search paths.
X Ownership issues with /usr/lib/audacious/Input.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

Comment 7 Michael Schwendt 2007-06-29 07:43:48 UTC
> X Ownership issues with /usr/lib/audacious/Input

Cannot confirm.
The base audacious package requires audacious-plugins.
This plugin package requires the base audacious package.

*However*, in general, any other multimedia software could
implement and use Audacious' plugins, so directory ownership is
not trivial to decide on. Probably it could only be fixed with
an audacious-base package or similar "bloat".

> BR pkgconfig

This package directly buildrequires pkgconfig for its m4 code.

Eliminating BR based on what some arbitrary package in the dep-chain
pulls in cannot be recommended, because e.g. audacious-devel may stop
using pkgconfig any time.


Comment 8 Jason Tibbitts 2007-06-29 14:24:52 UTC
> The base audacious package requires audacious-plugins.

Of course; I didn't chase the dependencies that far.

> This package directly buildrequires pkgconfig for its m4 code.

OK, that's not the usual usage, and I didn't read the actual code.

APPROVED

Comment 9 Michael Schwendt 2007-06-29 22:13:06 UTC
New Package CVS Request
=======================
Package Name: audacious-plugin-fc
Short Description: Future Composer input plugin for Audacious
Owners: bugs.michael
Branches: F-7
InitialCC: 


Comment 10 Kevin Fenzi 2007-07-02 18:39:23 UTC
cvs done.

Comment 11 Fedora Update System 2007-07-03 16:20:54 UTC
audacious-plugin-fc-0.2-1 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.

Comment 12 Fedora Update System 2007-07-05 19:24:33 UTC
audacious-plugin-fc-0.2-1 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.