Bug 822730

Summary: Review Request: laptop-mode-tools - Tools for power savings based on battery/AC status
Product: [Fedora] Fedora Reporter: Adrian Alves <aalves>
Component: Package ReviewAssignee: Jaroslav Škarvada <jskarvad>
Status: CLOSED INSUFFICIENT_DATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: acc-bugz-redhat, claudiozumbo, jskarvad, mrunge, package-review, pahan, rsarraf, sanjay.ankur, sergio, valent.turkovic, w00tw00t
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-06-07 12:29:14 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:
Attachments:
Description Flags
get spec from http://apt.sw.be/source/laptop-mode-tools-1.33-1.rf.src.rpm
none
tmpfiles config
none
Proposed systemd service file
none
1.63 spec file none

Description Adrian Alves 2012-05-18 01:50:59 UTC
Spec URL: http://alvesadrian.fedorapeople.org/laptop-mode-tools.spec
SRPM URL: http://alvesadrian.fedorapeople.org/laptop-mode-tools-1.61-2.fc16.src.rpm
Description: Laptop mode is a Linux kernel feature that allows your laptop to save considerable power, by allowing the hard drive to spin down for longer periods of time. This package contains the userland scripts that are needed to enable laptop mode. It includes support for automatically enabling laptop mode when the computer is working on batteries. In addition, it provides a set of modules which allow you to apply various other power savings.

Comment 1 Matthias Runge 2012-05-18 20:29:47 UTC
- fedora switched from sysv init to systemd in f16. You should take that into account, please provide a native service
- you're restarting acpid via service, but don't require acpid. Is that intentional? Does acpid work without acpid?

Comment 2 Adrian Alves 2012-05-19 18:35:29 UTC
(In reply to comment #1)
> - fedora switched from sysv init to systemd in f16. You should take that into
> account, please provide a native service
> - you're restarting acpid via service, but don't require acpid. Is that
> intentional? Does acpid work without acpid?

Removed the acpid restart from %post:
Spec URL: http://alvesadrian.fedorapeople.org/laptop-mode-tools.spec
SRPM URL: http://alvesadrian.fedorapeople.org/laptop-mode-tools-1.61-3.fc16.src.rpm

By the way how I can implement systemd for this? can u help me on that?

Comment 3 Sergio Basto 2012-05-25 01:56:21 UTC
Created attachment 586743 [details]
get spec from http://apt.sw.be/source/laptop-mode-tools-1.33-1.rf.src.rpm

I copy spec from rf (rpmforge) into this one .

Comment 4 Adrian Alves 2012-05-25 15:12:04 UTC
(In reply to comment #3)
> Created attachment 586743 [details]
> get spec from http://apt.sw.be/source/laptop-mode-tools-1.33-1.rf.src.rpm
> 
> I copy spec from rf (rpmforge) into this one .

I know i used the spec from dag I already said that. But i made the changes to make it works on fedora did u try it or make any review about it?

Comment 5 Jaroslav Škarvada 2012-05-28 00:14:55 UTC
Hi I took the review. As a member of Fedora Power Management SIG I am highly interested in such packages. Please give me some time to go through it in more detail (approx. one week).

Comment 6 Adrian Alves 2012-05-29 00:55:34 UTC
(In reply to comment #5)
> Hi I took the review. As a member of Fedora Power Management SIG I am highly
> interested in such packages. Please give me some time to go through it in
> more detail (approx. one week).
Sure take ur time can u help to migrate it to systemd?

Comment 7 Adrian Alves 2012-05-29 00:59:05 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Hi I took the review. As a member of Fedora Power Management SIG I am highly
> > interested in such packages. Please give me some time to go through it in
> > more detail (approx. one week).
> Sure take ur time can u help to migrate it to systemd?

Probably we can made some changes to make ir works into Fedora17 or 18 and rename it as fedora-laptop-tools 0r fedora-laptop-mode-tools. can we work it out together? I will need ur assistance to make it works on systemd. if u want contact me over gtalk mine is aalves at gmail.com

Comment 8 Sergio Basto 2012-06-04 17:26:27 UTC
(In reply to comment #4)

> I know i used the spec from dag 

Good, my diff is also based from dag 
 
> I already said that. But i made the changes
> to make it works on fedora did u try it or make any review about it?

1 - at least, put things in %install not in %build ( and leave %build empty) 

2 - %{__rm} -rf %{buildroot} is deprecated 

look at my diff, IMHO looks much more clean, I don't see the point of enumerate man and don't use %doc %{_mandir}/man8/*.8*

Comment 9 Matthias Runge 2012-06-06 09:46:43 UTC
some more comments:

you're dropping something to udev-dir. If that doesn't exist, installation will fail. You need to require udev.

Note: udev has been deprecated, see
http://lists.fedoraproject.org/pipermail/devel/2012-June/168227.html

Comment 10 Jaroslav Škarvada 2012-06-17 22:24:13 UTC
Currently I am on holidays with limited internet access, so my response time is a bit longer :) I will be back in office 2012-06-25.

Other things that I have spotted so far:

* I guess the license is GPLv2+, according to the text in /etc/apm/event.d/laptop-mode and also taking into account: https://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F
but probably the best would be to ask the author.

* Do not use macros for system commands, eg. %{__rm}, use rm instead:
http://fedoraproject.org/wiki/Packaging:Guidelines#Macros

* The %clean section or explicitly cleaning of the buildroot by 'rm -rf %{buildroot}' is not needed unless you are also packaging for EPEL-5 or lower:
http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean

* Escape macros in changelog by %, e.g. %%{__install}.

* Make man pages non-executable and compress them.

* Executables shouldn't be marked as config files.

* Also check all (S)RPMs with rpmlint, it seems there are some more problems:

rpmlint laptop-mode-tools-1.61-3.fc18.src.rpm

laptop-mode-tools.src: W: spelling-error %description -l en_US userland -> user land, user-land, slanderous
laptop-mode-tools.src: W: invalid-license GPL
laptop-mode-tools.src:21: W: setup-not-quiet
laptop-mode-tools.src:25: W: rpm-buildroot-usage %build %{__rm} -rf %{buildroot}
laptop-mode-tools.src:27: W: rpm-buildroot-usage %build DESTDIR=%{buildroot} INIT_D="" MAN_D=%{_mandir} INSTALL=install ./install.sh
laptop-mode-tools.src:30: W: rpm-buildroot-usage %build rm %{buildroot}/etc/init.d/laptop-mode
laptop-mode-tools.src:32: W: rpm-buildroot-usage %build %{__mkdir_p} -m0755 %{buildroot}%{_initrddir}
laptop-mode-tools.src:33: W: rpm-buildroot-usage %build %{__install} -Dp -m755 etc/init.d/laptop-mode %{buildroot}%{_initrddir}
laptop-mode-tools.src:70: E: hardcoded-library-path in /usr/lib/pm-utils/sleep.d/*
laptop-mode-tools.src:75: E: hardcoded-library-path in /usr/lib/pm-utils/sleep.d
laptop-mode-tools.src:133: W: macro-in-%changelog %{__install}
laptop-mode-tools.src: W: no-%install-section
1 packages and 0 specfiles checked; 2 errors, 10 warnings.


rpmlint laptop-mode-tools-1.61-3.fc18.noarch.rpm

laptop-mode-tools.noarch: W: manpage-not-compressed gz /usr/share/man/man8/laptop-mode.conf.8
laptop-mode-tools.noarch: W: manpage-not-compressed gz /usr/share/man/man8/lm-syslog-setup.8
laptop-mode-tools.noarch: W: manpage-not-compressed gz /usr/share/man/man8/lm-profiler.8
laptop-mode-tools.noarch: W: manpage-not-compressed gz /usr/share/man/man8/laptop_mode.8
laptop-mode-tools.noarch: W: manpage-not-compressed gz /usr/share/man/man8/lm-profiler.conf.8
laptop-mode-tools.noarch: W: spelling-error %description -l en_US userland -> user land, user-land, slanderous
laptop-mode-tools.noarch: W: invalid-license GPL
laptop-mode-tools.noarch: W: only-non-binary-in-usr-lib
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/events/lm_ac_adapter
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/actions/lm_ac_adapter.sh
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/events/lm_lid
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/actions/lm_lid.sh
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/rc.d/init.d/laptop-mode
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/events/lm_battery
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/acpi/actions/lm_battery.sh
laptop-mode-tools.noarch: W: conffile-without-noreplace-flag /etc/udev/rules.d/99-laptop-mode.rules
laptop-mode-tools.noarch: E: incorrect-fsf-address /etc/apm/event.d/laptop-mode
laptop-mode-tools.noarch: E: non-standard-executable-perm /usr/share/man/man8/laptop-mode.conf.8 0744L
laptop-mode-tools.noarch: W: spurious-executable-perm /usr/share/man/man8/laptop-mode.conf.8
laptop-mode-tools.noarch: E: executable-marked-as-config-file /etc/acpi/actions/lm_ac_adapter.sh
laptop-mode-tools.noarch: E: non-standard-executable-perm /usr/share/man/man8/lm-syslog-setup.8 0744L
laptop-mode-tools.noarch: W: spurious-executable-perm /usr/share/man/man8/lm-syslog-setup.8
laptop-mode-tools.noarch: E: executable-marked-as-config-file /etc/acpi/actions/lm_lid.sh
laptop-mode-tools.noarch: E: non-standard-executable-perm /usr/share/man/man8/lm-profiler.8 0744L
laptop-mode-tools.noarch: W: spurious-executable-perm /usr/share/man/man8/lm-profiler.8
laptop-mode-tools.noarch: E: non-standard-executable-perm /usr/share/man/man8/laptop_mode.8 0744L
laptop-mode-tools.noarch: W: spurious-executable-perm /usr/share/man/man8/laptop_mode.8
laptop-mode-tools.noarch: E: non-standard-executable-perm /usr/share/man/man8/lm-profiler.conf.8 0744L
laptop-mode-tools.noarch: W: spurious-executable-perm /usr/share/man/man8/lm-profiler.conf.8
laptop-mode-tools.noarch: E: incorrect-fsf-address /usr/share/doc/laptop-mode-tools-1.61/COPYING
laptop-mode-tools.noarch: E: executable-marked-as-config-file /etc/rc.d/init.d/laptop-mode
laptop-mode-tools.noarch: E: standard-dir-owned-by-package /usr/sbin
laptop-mode-tools.noarch: E: executable-marked-as-config-file /etc/acpi/actions/lm_battery.sh
laptop-mode-tools.noarch: E: subsys-not-used /etc/rc.d/init.d/laptop-mode
laptop-mode-tools.noarch: W: incoherent-init-script-name laptop-mode ('laptop-mode-tools', 'laptop-mode-toolsd')
1 packages and 0 specfiles checked; 13 errors, 22 warnings.


I think it is not good idea to rename the package to something like fedora-*, because AFAIK there is no conflict to resolve. We should stay as close to upstream as possible (all the changes that would make sense we should get upstream).

I will do the transition to systemd, no problem, stay tuned :)


Finally my personal point of view: 
AFAIK the laptop mode kernel feature is fading out as the SSDs are more and more common (I know that the tool can do much more, but this is presented as the core functionality in the docs). Also the power saving mechanism that depends on AC/battery mode is not sophisticated (e.g. some users may need full performance on battery to complete their task more quickly and then go to idle and save more power), so the shipped defaults may not be optimal for everybody.
<advertisement>
In Fedora we are working on Tuned project and we try to resolve this via profiles (also if required, the profiles can be switched automatically by /etc/pm/power.d script but we don't do it by default). If you are interested, contributions are welcome :) http://fedorahosted.org/tuned/
</advertisement>

Comment 11 Valent Turkovic 2012-10-12 10:25:55 UTC
Hi, any progress? Do you need testers for Fedora 17?

Comment 12 Jaroslav Škarvada 2012-10-17 12:01:19 UTC
(In reply to comment #9)
> you're dropping something to udev-dir. If that doesn't exist, installation
> will fail. You need to require udev.
> 
You will require even more, at least acpid and pm-utils.

You should probably also drop apm (preferred) or package support for it in i386 subpackage that will require apmd.

There is no pmud in Fedora, so you need to remove it (/etc/power stuff).

According to packaging guidelines [1] regarding the usrmove feature you shouldn't install to /lib, but /usr/lib.

Probably it cannot be noarch, because pm-utils isn't (pm-utils could be modified, but it is another story) and you cannot now blindly install to /usr/lib/pm-utils

[1] http://fedoraproject.org/wiki/Packaging:Guidelines#Filesystem_Layout

Comment 13 Jaroslav Škarvada 2012-10-17 12:05:17 UTC
Probably it should be also marked as conflicting with tuned.

Comment 14 Jaroslav Škarvada 2012-10-17 12:08:46 UTC
(In reply to comment #11)
> Hi, any progress? Do you need testers for Fedora 17?

Hi Valent, we need new SRPM. Then you can start testing, testers are always welcome :)

Comment 15 Jaroslav Škarvada 2012-10-17 12:33:59 UTC
Created attachment 628746 [details]
tmpfiles config

Comment 16 Jaroslav Škarvada 2012-10-17 12:35:41 UTC
Created attachment 628757 [details]
Proposed systemd service file

You need to remove sysvinit script or package it as sysvinit subpackage.

Comment 17 Jaroslav Škarvada 2012-10-17 12:42:46 UTC
Regarding systemd you also need to modify the SPEC according to:
http://fedoraproject.org/wiki/Packaging:Systemd#Packaging
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

Comment 18 Alvin Smith 2012-12-08 20:15:45 UTC
(In reply to comment #10)
> I will do the transition to systemd, no problem, stay tuned :)

I was wondering how this is coming along and if there is any way one could help to speed things up?

Comment 19 Jaroslav Škarvada 2012-12-10 08:37:35 UTC
(In reply to comment #18)
> (In reply to comment #10)
> > I will do the transition to systemd, no problem, stay tuned :)
> 
> I was wondering how this is coming along and if there is any way one could
> help to speed things up?

I guess, I am done, see comments 15, 16. Regarding 17 it's easy, only copy&paste from the provided links. But in case of trouble, I can also provide the patch.

We need new SRPM addressing the issues noted in previous comments.

Comment 20 Claudio 2013-05-14 19:24:27 UTC
Created attachment 747878 [details]
1.63 spec file

Spec file updated for 1.63

Comment 21 Claudio 2013-05-14 19:26:03 UTC
I've actully tried to fix a few things that had been mentioned in the older posts, let me know.

Comment 22 Jaroslav Škarvada 2013-05-27 10:11:01 UTC
(In reply to Claudio from comment #21)
> I've actully tried to fix a few things that had been mentioned in the older
> posts, let me know.

Comments:

* Please also provide the new SRPM, especially in case you rebased the package.

* Please add %install section [1]. Installing content from %build section by e.g.:
%{__install} -Dp -m755 etc/init.d/laptop-mode %{buildroot}%{_initrddir}
is unacceptable.

* In Fedora, we do not use the Packager tag, please drop it. Credits are already in the changelog, and the actual ownership of the package is better tracked in the pkgdb.

* Please use '%setup -q' [2]

* Macro forms of system executables SHOULD NOT be used... For example, rm should be used in preference to %{__rm} [3]

* Fedora (as of F-10) does not require the presence of the BuildRoot tag in the spec and if one is defined it will be ignored [4]. This tag is needed only if packaging/targeting the package to EPEL-5 (which is IMHO not this case). Also please do not explicitly clean the build root (i.e rm -rf %{buildroot}).

* Please use systemd macros to install the service file [5].

* Please do not use macros in changelog, prefix the macros by another %, i.e. use the following in the changelog:
- Restart acpid after %%{__install} -Dping.

* Please use macro %{_datadir} instead of %{_usr}/share.

* Please package initrd stuff to sysvinit subpackage.

* You msut not own the %{_sysconfdir}/acpi/ subdirs (events, actions), there are already owned by acpid package, you should require the acpid package.

* The same for apm.

* But you should probably own the %{_sysconfdir}/power directory.

* Please use Requires: pkg1, pkg2, ... (or leave the comma) to be consistent in formatting with other parameters instead of:
requires:udev
requires:acpid
...

* /sbin/chkconfig calls definitely belongs to the sysvinit subpackage [6].


[1] https://fedoraproject.org/wiki/How_to_create_an_RPM_package?rd=PackageMaintainers/CreatingPackageHowTo#.25install_section

[2] http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_.25setup_command

[3] http://fedoraproject.org/wiki/Packaging:Guidelines#Macros

[4] http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

[5] http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

[6] http://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_addition_to_systemd_unit_files

Comment 23 Jaroslav Škarvada 2013-05-27 10:13:08 UTC
* You must not own the %{_sysconfdir}/acpi/ subdirs (events, actions), there are already owned by acpid package, you should require the acpid package [1].

[1] http://fedoraproject.org/wiki/Packaging:ReviewGuidelines

Comment 24 Valent Turkovic 2015-01-10 20:42:29 UTC
This package has been dead in the water for really long time. Are laptop-tools tools compatible with systemd?

Are there any tools for swithcing laptop performance in current Fedora 21?

Comment 25 Valent Turkovic 2015-01-10 20:49:13 UTC
I found Fedora 20 documentation mention laptop-mode but in Fedora 21 documentation there is no mention of it:

https://docs.fedoraproject.org/en-US/Fedora/20/html/Power_Management_Guide/Example_Laptop.html

Comment 26 Valent Turkovic 2015-01-10 20:53:06 UTC
Also after building latest laptop-package rpm there is an error conflict with filesystem package.


rpmbuild -tb laptop-mode-tools_1.66.tar.gz 


sudo dnf install /home/valent/rpmbuild/RPMS/noarch/laptop-mode-tools-1.66-1.noarch.rpm

Install  1 Package

Total size: 110 k
Installed size: 346 k
Is this ok [y/N]: y
Downloading Packages:
Running transaction check
Transaction check succeeded.
Running transaction test
Error: Transaction check error:
  file /usr/sbin from install of laptop-mode-tools-1.66-1.noarch conflicts with file from package filesystem-3.2-28.fc21.x86_64

Error Summary

Comment 27 Matthias Runge 2016-06-07 12:29:14 UTC
IIRC, Adrian left the project some time ago.