Bug 511107 - Review Request: rhythmbox-equalizer - An Equalizer plugin for Rhythmbox
Summary: Review Request: rhythmbox-equalizer - An Equalizer plugin for Rhythmbox
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Thomas Kowaliczek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-13 17:22 UTC by Hicham HAOUARI
Modified: 2010-05-07 02:21 UTC (History)
6 users (show)

Fixed In Version: rhythmbox-equalizer-1.3-2.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-05-06 21:59:32 UTC
Type: ---
Embargoed:
linuxdonald: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Hicham HAOUARI 2009-07-13 17:22:11 UTC
Spec URL: http://hicham.iblogger.org/linux/fedora/releases/11/Everything/source/SPECS/rhythmbox-equalizer.spec
SRPM URL: http://hicham.iblogger.org/linux/fedora/releases/11/Everything/source/SRPMS/rhythmbox-equalizer-1.1-1.fc11.src.rpm
Description: 10 Band Equalizer for Rhythmbox

Koji Build Page :

http://koji.fedoraproject.org/koji/taskinfo?taskID=1471729

rpmlint output :

[hicham@localhost noarch]$ rpmlint -i rhythmbox-equalizer-1.1-1.fc11.noarch.rpm
rhythmbox-equalizer.noarch: W: only-non-binary-in-usr-lib
There are only non binary files in /usr/lib so they should be in /usr/share.

1 packages and 0 specfiles checked; 0 errors, 1 warnings.

[hicham@localhost SRPMS]$ rpmlint rhythmbox-equalizer-1.1-1.fc11.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

[hicham@localhost SPECS]$ rpmlint rhythmbox-equalizer.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Project Homepage : http://cornerofseven.com

Comment 1 Mads Kiilerich 2009-07-18 00:09:50 UTC
The macro up_name ... It isn't obvious to me what its name is short for, so it doesn't improve readability much. The value of the macro will probably never change, and the name of the macro is longer than its value. I suggest dropping it.

The source has no indication of the license. I also don't see any indication of the license on upstream site, so I can't confirm the GPL+ license. Upstream should be asked to state the license explicitly in the relased tar ball. And Rhythmbox is GPLv2+, so I don't think a GPL+ plugin like this(?) is legal?

The URL points to a blog covering many topics. If no real site exists then we should use for example a stable link to the announcement of this release.

ChangeLog and TODO ... The content of these files doesn't add much value to the package. If they were included in the upstream package they probably shouldn't be included in the package anyway. Supplying the files as extra sources without any indication of the origin makes it even more questionable. If anything then upstream should be asked to include the files in the tar-ball.

Rhythmbox already requires python, so requiring it here is not strictly necessary, but I guess that stating it explicitly is fine. But then it should also state all other requirements explicitly.

The source contains .pyc files - they are a kind of pre-built binaries and should be removed in %prep.

only-non-binary-in-usr-lib is caused by the location of rhythmboxs extension folder. There is nothing this package can do to fix it. BUT on x86 it uses /usr/lib/rhythmbox/plugins/rbeq while it is /usr/lib64/rhythmbox/plugins/rbeq on x86_64. So unfortunately this package isn't and can't be noarch.

I have successfully tested that the package works on x86.

--
[Looking for sponsor and review on bug 509936]

Comment 2 Hicham HAOUARI 2009-07-18 23:38:43 UTC
The macro up_name stands for upstream name.

You are right, upstream don't indicate any license yet, probably he will release it under GPLv2+.

The url is not precise, you are right.

Changelog and todo are taken from upstream website, I added them to suppress an rpmlint warning.

You are right, requiring python is not necessary.

You are right, pyc files should be removed.

Thanks for taking the time to review my package.

Comment 3 Hicham HAOUARI 2009-07-18 23:40:14 UTC
I am gonna update the spec and srpm soon

Comment 4 Hicham HAOUARI 2009-08-15 18:43:53 UTC
Updated, the spec file and srpm :

Spec URL : http://hicham.fedorapeople.org/rhythmbox-equalizer.spec
SRPM URL : http://hicham.fedorapeople.org/rhythmbox-equalizer-1.2-1.fc11.src.rpm

rpmlint output :
spec file : none
srpm file : nome
bin  rpm  :
[hicham@hicham i586]$ rpmlint rhythmbox-equalizer-1.2-1.fc11.i586.rpm
rhythmbox-equalizer.i586: E: no-binary
rhythmbox-equalizer.i586: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Comment 5 Thomas Kowaliczek 2010-03-28 23:58:36 UTC
Please update it too version 1.3 and i will review it.

Comment 6 Hicham HAOUARI 2010-04-12 21:10:08 UTC
There is a blocker to 1.3, i reported it upstream in : http://code.google.com/p/rbeq/issues/detail?id=6

I have three options now :

- Patch it to use gconf for presets
- Wait for upstream to do it
- Disable save button

I don't know which one to choose yet

Comment 7 Thomas Kowaliczek 2010-04-13 03:53:47 UTC
I think the best would be to wait what upstream say.

Comment 9 Thomas Kowaliczek 2010-05-01 12:13:40 UTC
MUST: rpmlint must be run on every package. The output should be posted in the review.
LinuxDonald@localhost SPECS]$ rpmlint /home/LinuxDonald/rpmbuild/RPMS/x86_64/rhythmbox-equalizer-1.3-2.fc12.x86_64.rpm
rhythmbox-equalizer.x86_64: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
rhythmbox-equalizer.x86_64: E: no-binary
rhythmbox-equalizer.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 2 warnings.
[LinuxDonald@localhost SPECS]$ 

MUST: The package must be named according to the  Package Naming Guidelines.
Okay

MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption.
Okay

MUST: The package must meet the  Packaging Guidelines.
Okay

MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines.
Okay

MUST: The License field in the package spec file must match the actual license.
Okay

MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.
Okay

MUST: The spec file must be written in American English.
Okay

MUST: The spec file for the package MUST be legible.
Okay

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the  Source URL Guidelines for how to deal with this.
Okay

MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
Okay

MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line.
Okay

MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
Okay

MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
Okay

MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
Okay

MUST: Packages must NOT bundle copies of system libraries.
Okay

MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker.
Okay

MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
Okay

MUST: A Fedora package must not list a file more than once in the spec file's %files listings.
Okay

MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
Okay

MUST: Each package must consistently use macros.
Okay

MUST: The package must contain code, or permissable content.
Okay

MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity).
Okay

MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present.
Okay

MUST: Header files must be in a -devel package.
Okay

MUST: Static libraries must be in a -static package.
Okay

MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
Okay

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}.
Okay

MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
Okay

MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation.
Okay

MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time.
Okay

MUST: All filenames in rpm packages must be valid UTF-8.
Okay

Comment 10 Thomas Kowaliczek 2010-05-01 12:14:20 UTC
That´s the only what i found maybe you can fix it?

MUST: rpmlint must be run on every package. The output should be posted in the review.
LinuxDonald@localhost SPECS]$ rpmlint /home/LinuxDonald/rpmbuild/RPMS/x86_64/rhythmbox-equalizer-1.3-2.fc12.x86_64.rpm
rhythmbox-equalizer.x86_64: W: spelling-error Summary(en_US) plugin -> plug in, plug-in, plugging
rhythmbox-equalizer.x86_64: E: no-binary
rhythmbox-equalizer.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 2 warnings.
[LinuxDonald@localhost SPECS]$

When not it is okay too.

Comment 11 Hicham HAOUARI 2010-05-01 12:27:05 UTC
(In reply to comment #10)
> That´s the only what i found maybe you can fix it?
> 
> MUST: rpmlint must be run on every package. The output should be posted in the
> review.
> LinuxDonald@localhost SPECS]$ rpmlint
> /home/LinuxDonald/rpmbuild/RPMS/x86_64/rhythmbox-equalizer-1.3-2.fc12.x86_64.rpm
> rhythmbox-equalizer.x86_64: W: spelling-error Summary(en_US) plugin -> plug in,
> plug-in, plugging

the word "Plugin" is used like that within rhythmbox

> rhythmbox-equalizer.x86_64: E: no-binary
> rhythmbox-equalizer.x86_64: W: only-non-binary-in-usr-lib

This package would have been ideally noarch, but since it installs files in arch dependant directories, there is no way to remove those warnings.

> 1 packages and 0 specfiles checked; 1 errors, 2 warnings.
> [LinuxDonald@localhost SPECS]$
> 
> When not it is okay too.

Comment 12 Thomas Kowaliczek 2010-05-01 13:21:44 UTC
Than it´s aporrvend by me i don´t found any problems with it.

Comment 13 Mads Kiilerich 2010-05-01 13:38:34 UTC
(In reply to comment #11)
> > rhythmbox-equalizer.x86_64: E: no-binary
> > rhythmbox-equalizer.x86_64: W: only-non-binary-in-usr-lib
> 
> This package would have been ideally noarch, but since it installs files in
> arch dependant directories, there is no way to remove those warnings.

Doesn't that mean that it is a bug in rhythmbox that it doesn't have an arch-independent plugin directory? (I would expect it do something similar to how x86_64 python uses both /usr/lib/python2.6/site-packages and /usr/lib64/python2.6/site-packages.)

Let's make Fedora ideal - and be pragmatic while we try ;-)

Comment 14 Hicham HAOUARI 2010-05-01 13:48:32 UTC
(In reply to comment #13)
> (In reply to comment #11)
> > > rhythmbox-equalizer.x86_64: E: no-binary
> > > rhythmbox-equalizer.x86_64: W: only-non-binary-in-usr-lib
> > 
> > This package would have been ideally noarch, but since it installs files in
> > arch dependant directories, there is no way to remove those warnings.
> 
> Doesn't that mean that it is a bug in rhythmbox that it doesn't have an
> arch-independent plugin directory? (I would expect it do something similar to
> how x86_64 python uses both /usr/lib/python2.6/site-packages and
> /usr/lib64/python2.6/site-packages.)
> 
> Let's make Fedora ideal - and be pragmatic while we try ;-)    

This is beyond the scope of this ticket.

Comment 15 Hicham HAOUARI 2010-05-01 13:50:46 UTC
New Package CVS Request
=======================
Package Name: rhythmbox-equalizer
Short Description: An Equalizer Plugin for Rhythmbox
Owners: hicham
Branches: F-11 F-12 F-13
InitialCC: hicham

Comment 16 Hicham HAOUARI 2010-05-01 13:51:17 UTC
and thanks Thomas for reviewing this package

Comment 17 Christoph Wickert 2010-05-01 13:58:34 UTC
(In reply to comment #14)
> This is beyond the scope of this ticket.    

Indeed, but as you maintain a plugin that might benefit from this change, you could take care of this.

Arch independent plugins should go to /usr/share/rhythmobx/plugins, just like
in Firefox. Filed as https://bugzilla.gnome.org/show_bug.cgi?id=617373
upstream. Please CC yourself to that bug.

Comment 18 Hicham HAOUARI 2010-05-01 14:03:57 UTC
Thanks Christoph for helping this review.

Comment 19 Christoph Wickert 2010-05-01 14:06:21 UTC
No problem, you are welcome.

BTW: The license tag is wrong, should be GPLv2+ instead of GPLv2, see http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#.22or_later_version.22_licenses

Comment 20 Hicham HAOUARI 2010-05-01 14:12:05 UTC
thanks Christoph, i will fix this upon initial import

Comment 21 Thomas Kowaliczek 2010-05-01 15:53:50 UTC
Thank you Cwickert :) You know i´m not the best reviewer ....

Comment 22 Kevin Fenzi 2010-05-04 02:43:31 UTC
CVS done (by process-cvs-requests.py).

Comment 23 Fedora Update System 2010-05-04 21:24:14 UTC
rhythmbox-equalizer-1.3-2.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/rhythmbox-equalizer-1.3-2.fc13

Comment 24 Fedora Update System 2010-05-04 21:27:16 UTC
rhythmbox-equalizer-1.3-2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/rhythmbox-equalizer-1.3-2.fc12

Comment 25 Fedora Update System 2010-05-04 21:29:03 UTC
rhythmbox-equalizer-1.3-2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rhythmbox-equalizer-1.3-2.fc11

Comment 26 Fedora Update System 2010-05-05 07:25:01 UTC
rhythmbox-equalizer-1.3-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 27 Fedora Update System 2010-05-06 03:39:40 UTC
rhythmbox-equalizer-1.3-2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 28 Fedora Update System 2010-05-06 03:42:41 UTC
rhythmbox-equalizer-1.3-2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 29 sawrub 2010-05-06 18:00:45 UTC
Rhythmbox fails to Quit with the plugin enabled. Besides that the plugin is working fine.

Installed:
  rhythmbox-equalizer.x86_64 0:1.3-2.fc12

Comment 30 sawrub 2010-05-06 18:06:49 UTC
Though disabling the plugin all works fine.

Comment 31 Hicham HAOUARI 2010-05-06 18:07:26 UTC
try starting rhythmbox from the console and then try quitting and grab any useful output in case of quit failure.

i don't have this issue in here

Comment 32 Hicham HAOUARI 2010-05-06 18:10:52 UTC
also if you have two copies of the plugin ( ie system-wide and user one ) try removing the user plugin as it may cause trouble.

Comment 33 Christoph Wickert 2010-05-06 21:59:32 UTC
This is not the place to discuss bugs, this is the review only. Please file a new bug against the proper package because people will hardly find a bug in rhythmbox-equalizer in the 'Package Review' component.

BTW: I can quit rythmbox with the plugin enabled just fine, even if the equalizer windows is opened.

Comment 34 sawrub 2010-05-07 02:21:16 UTC
@Christoph :
Ok Have filed bug : https://bugzilla.redhat.com/show_bug.cgi?id=589826 to take care of the issue.

@Hicham : 
There is only single copy of the plugin installed, via YUM.
Also running and quiting rhythmbox from Console is working fine, its just through GUI the issue is with.


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