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
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]
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.
I am gonna update the spec and srpm soon
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.
Please update it too version 1.3 and i will review it.
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
I think the best would be to wait what upstream say.
Created a small patch to workaround the problem http://hicham.fedorapeople.org/rhythmbox-equalizer-1.3-fix-presets-per-user.patch Spec URL : http://hicham.fedorapeople.org/rhythmbox-equalizer.spec SRPM URL : http://hicham.fedorapeople.org/rhythmbox-equalizer-1.3-2.fc13.src.rpm
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
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.
(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.
Than it´s aporrvend by me i don´t found any problems with it.
(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 ;-)
(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.
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
and thanks Thomas for reviewing this package
(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.
Thanks Christoph for helping this review.
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
thanks Christoph, i will fix this upon initial import
Thank you Cwickert :) You know i´m not the best reviewer ....
CVS done (by process-cvs-requests.py).
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
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
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
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.
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.
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.
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
Though disabling the plugin all works fine.
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
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.
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.
@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.