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 Review | Assignee: | Jaroslav Škarvada <jskarvad> | ||||||||||
Status: | CLOSED INSUFFICIENT_DATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||||||||
Severity: | medium | Docs Contact: | |||||||||||
Priority: | medium | ||||||||||||
Version: | rawhide | CC: | 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
Adrian Alves
2012-05-18 01:50:59 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? (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? 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 . (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? 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). (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? (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 (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* 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 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> Hi, any progress? Do you need testers for Fedora 17? (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 Probably it should be also marked as conflicting with tuned. (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 :) Created attachment 628746 [details]
tmpfiles config
Created attachment 628757 [details]
Proposed systemd service file
You need to remove sysvinit script or package it as sysvinit subpackage.
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 (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? (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. Created attachment 747878 [details]
1.63 spec file
Spec file updated for 1.63
I've actully tried to fix a few things that had been mentioned in the older posts, let me know. (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 * 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 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? 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 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 IIRC, Adrian left the project some time ago. |