Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: laptop-mode-tools - Tools for power savings based on battery/AC status
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Summary: Review Request: laptop-mode-tools - Tools for power savings based on battery/AC status Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: aalves@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: --- Regression: --- Mount Type: --- Documentation: ---
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Matthias Runge mrunge@matthias-runge.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mrunge@matthias-runge.de
--- Comment #1 from Matthias Runge mrunge@matthias-runge.de 2012-05-18 16:29:47 EDT --- - 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?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #2 from Adrian Alves aalves@gmail.com 2012-05-19 14:35:29 EDT --- (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?
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Jaroslav Škarvada jskarvad@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |jskarvad@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Jaroslav Škarvada jskarvad@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |jskarvad@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #3 from Sergio Monteiro Basto sergio@serjux.com --- Created attachment 586743 --> https://bugzilla.redhat.com/attachment.cgi?id=586743&action=edit 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 .
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #4 from Adrian Alves aalves@gmail.com --- (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?
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #5 from Jaroslav Škarvada jskarvad@redhat.com --- 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).
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #6 from Adrian Alves aalves@gmail.com --- (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?
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #7 from Adrian Alves aalves@gmail.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Sergio Monteiro Basto sergio@serjux.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |sergio@serjux.com
--- Comment #8 from Sergio Monteiro Basto sergio@serjux.com --- (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*
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #9 from Matthias Runge mrunge@matthias-runge.de --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #10 from Jaroslav Škarvada jskarvad@redhat.com --- 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_versio... 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>
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Valent Turkovic valent.turkovic@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |valent.turkovic@gmail.com Flags| |needinfo?(jskarvad@redhat.c | |om)
--- Comment #11 from Valent Turkovic valent.turkovic@gmail.com --- Hi, any progress? Do you need testers for Fedora 17?
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Jaroslav Škarvada jskarvad@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(jskarvad@redhat.c | |om) |
--- Comment #12 from Jaroslav Škarvada jskarvad@redhat.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #13 from Jaroslav Škarvada jskarvad@redhat.com --- Probably it should be also marked as conflicting with tuned.
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #14 from Jaroslav Škarvada jskarvad@redhat.com --- (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 :)
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #15 from Jaroslav Škarvada jskarvad@redhat.com --- Created attachment 628746 --> https://bugzilla.redhat.com/attachment.cgi?id=628746&action=edit tmpfiles config
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #16 from Jaroslav Škarvada jskarvad@redhat.com --- Created attachment 628757 --> https://bugzilla.redhat.com/attachment.cgi?id=628757&action=edit Proposed systemd service file
You need to remove sysvinit script or package it as sysvinit subpackage.
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Jaroslav Škarvada jskarvad@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #628757|application/octet-stream |text/plain mime type| |
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #17 from Jaroslav Škarvada jskarvad@redhat.com --- 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
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=822730
Alvin Smith w00tw00t@bobmail.info changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |w00tw00t@bobmail.info
--- Comment #18 from Alvin Smith w00tw00t@bobmail.info --- (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?
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #19 from Jaroslav Škarvada jskarvad@redhat.com --- (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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #20 from Claudio claudiozumbo@gmail.com --- Created attachment 747878 --> https://bugzilla.redhat.com/attachment.cgi?id=747878&action=edit 1.63 spec file
Spec file updated for 1.63
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=822730
Claudio claudiozumbo@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |claudiozumbo@gmail.com
--- Comment #21 from Claudio claudiozumbo@gmail.com --- I've actully tried to fix a few things that had been mentioned in the older posts, let me know.
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #22 from Jaroslav Škarvada jskarvad@redhat.com --- (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=PackageMainta...
[2] http://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25prep_section:_...
[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_additi...
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #23 from Jaroslav Škarvada jskarvad@redhat.com --- * 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
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #24 from Valent Turkovic valent.turkovic@gmail.com --- 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?
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #25 from Valent Turkovic valent.turkovic@gmail.com --- 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/E...
https://bugzilla.redhat.com/show_bug.cgi?id=822730
--- Comment #26 from Valent Turkovic valent.turkovic@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=822730
Matthias Runge mrunge@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution|--- |INSUFFICIENT_DATA Last Closed| |2016-06-07 08:29:14
--- Comment #27 from Matthias Runge mrunge@redhat.com --- IIRC, Adrian left the project some time ago.
package-review@lists.fedoraproject.org