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 222648 - Review Request: audacious-plugin-fc - Future Composer plugin for Audacious
Summary: Review Request: audacious-plugin-fc - Future Composer plugin for Audacious
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-15 14:46 UTC by Michael Schwendt
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version: 0.2-1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-07-05 19:24:36 UTC
Type: ---
Embargoed:
j: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

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.


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