Bug 688408 - Review Request: xfce4-cpufreq-plugin - CPU frequency scaling plugin for the Xfce4 panel
Summary: Review Request: xfce4-cpufreq-plugin - CPU frequency scaling plugin for the X...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-16 23:31 UTC by Hicham HAOUARI
Modified: 2018-05-28 17:53 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-05-28 17:53:00 UTC
mario.blaettermann: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Hicham HAOUARI 2011-03-16 23:31:06 UTC
Spec URL: http://hicham.fedorapeople.org/xfce4-cpufreq-plugin/xfce4-cpufreq-plugin.spec
SRPM URL: http://hicham.fedorapeople.org/xfce4-cpufreq-plugin/xfce4-cpufreq-plugin-1.0.0-1.fc15.src.rpm
Description:
The CpuFreq Plugin shows in the Xfce Panel the following information:
    current CPU frequency
    current used governor

In a separate dialog it provides you following information:
    all available CPU frequencies
    all available governors
    used driver for the CPU

Comment 1 Mario Blättermann 2011-03-17 21:38:05 UTC
Some issues:

Because your package is using the folder

%{_datadir}/icons/hicolor/

you should add the following line:

Requires: hicolor-icon-theme

This installation (not runtime!) dependency won't picked up automatically.


Moreover, no group is defined. I would recommend »User Interface/Desktops«, as usual for other Xfce plugins.

Please use macros consistently. Replace $RPM_BUILD_ROOT with %{buildroot}.

Comment 2 Hicham HAOUARI 2011-03-18 20:05:22 UTC
1) It is useless to requires hicolor-icon-theme as gtk2 already requires it
2) Right, I forgot to add the group since the spec was created from the standard template for rpm >= 4.9, maybe I should file a bug against rpmdevtools.
3) $RPM_BUILD_ROOT is standard in all specs, what is not allowed is mixing it with %{buildroot}

Comment 3 Hicham HAOUARI 2011-03-18 20:14:47 UTC
Updated, no release increment

Thanks Mario for helping with this review

Comment 4 Mario Blättermann 2011-03-20 12:18:40 UTC
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=2926356


rpmlint output:

$ rpmlint -i -v xfce4-c*src.rpm
xfce4-cpufreq-plugin.src: I: checking
xfce4-cpufreq-plugin.src: I: checking-url http://goodies.xfce.org/projects/panel-plugins/xfce4-cpufreq-plugin (timeout 10 seconds)
xfce4-cpufreq-plugin.src: I: checking-url http://archive.xfce.org/src/panel-plugins/xfce4-cpufreq-plugin/1.0/xfce4-cpufreq-plugin-1.0.0.tar.bz2 (timeout 10 seconds)
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

OK.

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    GPLv2+
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: The file containing the text of the license(s) for the package must
be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum xfce4-cpufreq-plugin*
    24cae9b8583cae82b715b4f72aa8e341  xfce4-cpufreq-plugin-1.0.0.tar.bz2
    24cae9b8583cae82b715b4f72aa8e341  xfce4-cpufreq-plugin-1.0.0.tar.bz2.packaged

[+] MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture.
- See Koji build above.
[.] MUST: If the package does not successfully compile, build or work on an
architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: The spec file MUST handle locales properly.
[+] MUST: If a package installs files below %{_datadir}/icons, the icon cache
must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call
ldconfig in %post and %postun.
[.] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[+] MUST: Permissions on files must be set properly.
[+] MUST: Packages must not provide RPM dependency information when that
information is not global in nature, or are otherwise handled.
[.] MUST: When filtering automatically generated RPM dependency information,
the filtering system implemented by Fedora must be used.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned
dependency.
[.] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop
file
[+] MUST: .desktop files must be properly installed with desktop-file-install
in the %install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.

[.] SHOULD: If the source package does not include license text(s) as a
    separate file from upstream, the packager SHOULD query upstream...

[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
    See Koji build above (which uses mock anyway)
[+] SHOULD: The reviewer should test that the package functions as described.
    Works properly on my machine (x86, xfce-4.6.x)
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin ...
[.] SHOULD: Your package should contain man pages for binaries/scripts.
    Currently no man page available.

--------------------------------------

PACKAGE APPROVED

--------------------------------------

Comment 5 Hicham HAOUARI 2011-03-21 02:20:48 UTC
Thanks Mario for reviewing this package

Comment 6 Hicham HAOUARI 2011-03-21 02:22:07 UTC
New Package SCM Request
=======================
Package Name: xfce4-cpufreq-plugin
Short Description: CPU frequency scaling plugin for the Xfce4 panel
Owners: hicham
Branches: f13 f14 f15
InitialCC: hicham

Comment 7 Jason Tibbitts 2011-03-21 13:45:34 UTC
Git done (by process-git-requests).

Comment 8 Fedora Update System 2011-03-21 23:36:59 UTC
xfce4-cpufreq-plugin-1.0.0-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/xfce4-cpufreq-plugin-1.0.0-1.fc15

Comment 9 Fedora Update System 2011-03-21 23:37:22 UTC
xfce4-cpufreq-plugin-1.0.0-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/xfce4-cpufreq-plugin-1.0.0-1.fc14

Comment 10 Fedora Update System 2011-03-21 23:37:28 UTC
xfce4-cpufreq-plugin-1.0.0-1.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/xfce4-cpufreq-plugin-1.0.0-1.fc13

Comment 11 Fedora Update System 2011-03-22 03:29:57 UTC
xfce4-cpufreq-plugin-1.0.0-1.fc15 has been pushed to the Fedora 15 testing repository.

Comment 12 Christoph Wickert 2011-03-22 15:59:10 UTC
Too bad I wasn't aware of this review. Next time you package something for Xfce, please ping the Xfce SIG.

I packaged this plugin earlier, but we agreed to not include it in Fedora for several reasons:
* Usually the power managers take care of scaling the CPU frequency.
* The plugin does not work with Fedora's default setup. Scaling is only allowed for root or through PolicyKit, but the plugin doesn't support the latter. How did you make the plugin work?
* There usually is no need for changing frequencies or the governor, ondemand will handle this best.

Some notes on the package:
1) The description is formatted a little strange
2) "Requires: hicolor-icon-theme" is not needed because the plugin requires gtk2 which already has a dependency to hicolor-icon-theme.
3) The Requires for xfce4-panel should be versioned and the version should be adjusted for the different Fedora releases. A plugin built for Xfce4-panel xfce4-panel 4.8 will not work with 4.6 or 4.4, even though the srpm is the same.

Comment 13 Hicham HAOUARI 2011-03-22 16:14:26 UTC
(In reply to comment #12)
> Too bad I wasn't aware of this review. Next time you package something for
> Xfce, please ping the Xfce SIG.

Right, I talked to nirik before submitting it.

> 
> I packaged this plugin earlier, but we agreed to not include it in Fedora for
> several reasons:
> * Usually the power managers take care of scaling the CPU frequency.

Right
> * The plugin does not work with Fedora's default setup. Scaling is only allowed
> for root or through PolicyKit, but the plugin doesn't support the latter. How
> did you make the plugin work?

I missed that, I will work with upstream to add polkit support ASAP.

> * There usually is no need for changing frequencies or the governor, ondemand
> will handle this best.

I don't agree, under certain conditions I want to force the cpu to use a certain frequency to keep my laptop cool.

> 
> Some notes on the package:
> 1) The description is formatted a little strange

Define "strange".
> 2) "Requires: hicolor-icon-theme" is not needed because the plugin requires
> gtk2 which already has a dependency to hicolor-icon-theme.

Right

> 3) The Requires for xfce4-panel should be versioned and the version should be
> adjusted for the different Fedora releases. A plugin built for Xfce4-panel
> xfce4-panel 4.8 will not work with 4.6 or 4.4, even though the srpm is the
> same.

I will work on that



Thanks Christoph for helping with this review

Comment 14 Christoph Wickert 2011-03-22 20:46:57 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > * The plugin does not work with Fedora's default setup. Scaling is only allowed
> > for root or through PolicyKit, but the plugin doesn't support the latter. How
> > did you make the plugin work?
> 
> I missed that, I will work with upstream to add polkit support ASAP.

Thanks, but does the plugin work for you? I tried it and it didn't and as long as it does not work, it shouldn't be in the repo.

> > * There usually is no need for changing frequencies or the governor, ondemand
> > will handle this best.
> 
> I don't agree, under certain conditions I want to force the cpu to use a
> certain frequency to keep my laptop cool.

I don't disagree, but we were told so by the kernel and power management people.

> > Some notes on the package:
> > 1) The description is formatted a little strange
> 
> Define "strange".

The indented lines look strange, better use * as bullets or markdown (will give you proper bullet points in packagekit)

Comment 15 Mario Blättermann 2013-06-02 15:04:33 UTC
What to do with this quite old ticket? The packaged hasn't reached the stable repos yet, and I don't expect it in the future. Should we close it as "closed/cantfix" or anything similar?

Comment 16 Mukundan Ragavan 2015-03-27 01:07:40 UTC
Package Change Request
======================
Package Name: xfce4-cpufreq-plugin
New Branches: epel7
Owners: nonamedotc hicham
InitialCC: nonamedotc

Comment 17 Gwyn Ciesla 2015-03-27 12:30:02 UTC
Git done (by process-git-requests).

Comment 18 Upstream Release Monitoring 2015-12-04 02:39:58 UTC
pbrobinson's scratch build of xfce4-cpugraph-plugin?#c58cb8c04e672a7a0f02babbc540fd072b0aa20a for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-cpugraph-plugin?#c58cb8c04e672a7a0f02babbc540fd072b0aa20a failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039568

Comment 19 Upstream Release Monitoring 2015-12-04 02:40:26 UTC
pbrobinson's scratch build of xfce4-cpufreq-plugin?#0840bc18587d2841e8bbc517b75c2f2fd2a7a3f9 for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-cpufreq-plugin?#0840bc18587d2841e8bbc517b75c2f2fd2a7a3f9 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039564

Comment 20 Upstream Release Monitoring 2015-12-04 02:40:37 UTC
pbrobinson's scratch build of xfce4-equake-plugin?#c50bc30c54cd79d58a11c8d6896b6c9b61cccdf8 for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-equake-plugin?#c50bc30c54cd79d58a11c8d6896b6c9b61cccdf8 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039574

Comment 21 Upstream Release Monitoring 2015-12-04 02:40:41 UTC
pbrobinson's scratch build of xfce4-fsguard-plugin?#c76e41d6f20c5291a137f4b1ac433812e59d43e3 for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-fsguard-plugin?#c76e41d6f20c5291a137f4b1ac433812e59d43e3 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039578

Comment 22 Upstream Release Monitoring 2015-12-04 02:40:51 UTC
pbrobinson's scratch build of xfce4-battery-plugin?#c4f924bc7288dade48e82d7af67768190c276434 for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-battery-plugin?#c4f924bc7288dade48e82d7af67768190c276434 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039563

Comment 23 Upstream Release Monitoring 2015-12-04 02:41:19 UTC
pbrobinson's scratch build of xfce4-dev-tools?#0d18d8715deacbb8d806184fe8bd672339d1d836 for epel7-archbootstrap and git://pkgs.fedoraproject.org/xfce4-dev-tools?#0d18d8715deacbb8d806184fe8bd672339d1d836 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12039571


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