Fedora Merge Review: pm-utils http://cvs.fedora.redhat.com/viewcvs/devel/pm-utils/ Initial Owner: pknirsch
I would be happy to review this package. Look for a full review in a bit.
OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPL) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. See below - Sources match upstream md5sum: See below - BuildRequires correct OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. See below - No rpmlint output. See below - final provides and requires are sane: SHOULD Items: OK - Should build in mock. OK - Should build on all supported archs OK - Should have dist tag OK - Should package latest version 15 outstanding bugs - check for outstanding bugs on package. Issues: 1. Is there no upstream repo for this package? Perhaps you could add it to 'hosted.fedoraproject.org' so it has some upstream presense? Also upstream for radeontool and vbetool links would be nice. 2. Why is pkgconfig BuildRequires there? 3. Should there be a Requires: pam? 4. Our rpmlint friend says: rpmlint on pm-utils-0.19.1-6.fc7.src.rpm W: pm-utils no-url-tag Would be nice to have upstream. W: pm-utils strange-permission 60sysfont.hook 0755 W: pm-utils strange-permission 65sound.hook 0755 I think thats ok. W: pm-utils unversioned-explicit-obsoletes vbetool W: pm-utils unversioned-explicit-provides vbetool W: pm-utils unversioned-explicit-obsoletes radeontool Would be very nice to provides versions on these if they are split out later. rpmlint on pm-utils-0.19.1-6.fc7.x86_64.rpm W: pm-utils no-url-tag W: pm-utils symlink-should-be-relative /etc/sysconfig/power-management /etc/pm/config Should make a relative symlink there. E: pm-utils executable-marked-as-config-file /etc/pm/config E: pm-utils script-without-shebang /etc/pm/config Should be mode 644 W: pm-utils non-conffile-in-etc /etc/security/console.apps/pm-hibernate W: pm-utils non-conffile-in-etc /etc/pam.d/pm-hibernate W: pm-utils non-conffile-in-etc /etc/pam.d/pm-suspend W: pm-utils non-conffile-in-etc /etc/pam.d/pm-powersave W: pm-utils non-conffile-in-etc /etc/security/console.apps/pm-suspend W: pm-utils non-conffile-in-etc /etc/security/console.apps/pm-powersave I think these can't be avoided, but should perhaps be config(noreplace). W: pm-utils non-conffile-in-etc /etc/pm/hooks/49bluetooth E: pm-utils non-executable-script /etc/pm/hooks/49bluetooth 0644 Should remove the #!/bin/bash there. W: pm-utils dangerous-command-in-%pre cp What are you trying to do in that pre? It looks odd. 5. Why is there a Conflicts: bluez-utils < 2.25-6 ? Shouldn't you just require the newer one? 6. Should radeontool and vbetool be split out? 7. Should use smp_mflags? 8. Should check the 15 outstanding bugs.
Per the new review procedure: https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html Setting fedora-review to ? and needinfo.
The spec also still mentions "Fedora Core".
(In reply to comment #2) > 1. Is there no upstream repo for this package? > Perhaps you could add it to 'hosted.fedoraproject.org' so it has some upstream > presense? There is http://webcvs.freedesktop.org/pm-utils/pm-utils/ but I did not yet find any source for tarballs. > Also upstream for radeontool and vbetool links would be nice. I guess for vbetool there is only the directory where the source comes from, for radeontoll I added something to the spec. > rpmlint on pm-utils-0.19.1-6.fc7.src.rpm > W: pm-utils no-url-tag > > Would be nice to have upstream. I added http://pm-utils.freedesktop.org/, where it is planned to add some information according to the pm-utils mailinglist. > W: pm-utils unversioned-explicit-obsoletes vbetool > W: pm-utils unversioned-explicit-provides vbetool > W: pm-utils unversioned-explicit-obsoletes radeontool > > Would be very nice to provides versions on these if they are split out later. Would this be better? Obsoletes: vbetool < 0.7-0 Provides: vbetool = 0.7-0 Obsoletes: radeontool < 1.5-0 Provides: radeontool = 1.5-0 I will look into the other issues later, but it may need some time.
Package Change Request ====================== Package Name: pm-utils Updated Description: Power management utilities and scripts for Fedora
cvs done.
radeontool and vbetool are split out now.
Hey Till. Any chance we can revisit the remaining items and get this review finished off? If you prefer I can re-review the current rawhide spec, just let me know...
Any chance of finishing off the last items here and closing this one out?
There is a lot of this fixed in Rawhide. Pm-utils just got a new release and upstream maintainer, therefore I want to fix everything else when I include this release, which will probably take some time, because I do not have so much currently and also it is too late to include this into F9.
Afaics these are the issues, that still need to be addressed, I will see what I can do about them soon: 2. Why is pkgconfig BuildRequires there? 3. Should there be a Requires: pam? 5. Why is there a Conflicts: bluez-utils < 2.25-6 ? Shouldn't you just require the newer one? pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-suspend-hybrid pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-hibernate /usr/lib/pm-utils/bin/pm-action pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-hibernate pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-hibernate pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-suspend-hybrid pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-suspend pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-suspend-hybrid /usr/lib/pm-utils/bin/pm-action pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-powersave pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-suspend /usr/lib/pm-utils/bin/pm-action pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-suspend pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-powersave pm-utils.i686: W: log-files-without-logrotate /var/log/pm-suspend.log pm-utils.i686: W: dangerous-command-in-%pre mv pm-utils.i686: W: dangerous-command-in-%post mv What are you trying to do in that pre? It looks odd.
(In reply to comment #12) > 2. Why is pkgconfig BuildRequires there? This will be removed (also all other BRs). Maybe they were all needed for vbetool or radeontool, which now are in their own packages. > 3. Should there be a Requires: pam? pm-utils requires usermode which requires pam. > 5. Why is there a > Conflicts: bluez-utils < 2.25-6 > ? Shouldn't you just require the newer one? This will be removed, too. > pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-hibernate > /usr/lib/pm-utils/bin/pm-action > pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-suspend-hybrid > /usr/lib/pm-utils/bin/pm-action > pm-utils.i686: W: symlink-should-be-relative /usr/sbin/pm-suspend > /usr/lib/pm-utils/bin/pm-action I do not know how to do this directly in automake, I will see what we can do about this at upstream. > pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-suspend-hybrid > pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-hibernate > pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-hibernate > pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-suspend-hybrid > pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-suspend > pm-utils.i686: W: non-conffile-in-etc /etc/pam.d/pm-powersave > pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-suspend > pm-utils.i686: W: non-conffile-in-etc /etc/security/console.apps/pm-powersave I am not sure, whether these file are intended to be edited by anyone, half of them are empty, anyways. Maybe it is even completely wrong to allow users to run pm-utils directly, so these files can be removed. > pm-utils.i686: W: log-files-without-logrotate /var/log/pm-suspend.log The logfile will be emptied on every run, so there is no need to rotate it. > pm-utils.i686: W: dangerous-command-in-%pre mv > pm-utils.i686: W: dangerous-command-in-%post mv > > What are you trying to do in that pre? It looks odd. The scriptlets move the old config files to the new locations, I guess they can be removed when F9 is branched, because then every release should already have had the new pm-utils.
On issue is missing: it needs to be checked, whether these Requires are needed: kbd pciutils >= 2.2.1
Excellent. Can you ping me again when you are ready for me to recheck things? Thanks for looking into it.
Everything except the rpmlint warnings and the Requires-check is now done, therefore a recheck would already help.
The list of isses is now down to: - symlinks - Requires: kbd pciutils >= 2.2.1 - %pre/%post scriptlits
Sorry for the long delay here. ;( rpmlint now says: pm-utils.src: W: mixed-use-of-spaces-and-tabs (spaces: line 39, tab: line 72) Fix if you like.. not a big deal. pm-utils.src: W: strange-permission pm-utils-99hd-apm-restore 0755 pm-utils.src: W: strange-permission pm-utils-bugreport-info.sh 0755 ignore. pm-utils.x86_64: W: symlink-should-be-relative /usr/sbin/pm-hibernate /usr/lib64/pm-utils/bin/pm-action pm-utils.x86_64: W: symlink-should-be-relative /usr/sbin/pm-suspend-hybrid /usr/lib64/pm-utils/bin/pm-action pm-utils.x86_64: W: symlink-should-be-relative /usr/sbin/pm-suspend /usr/lib64/pm-utils/bin/pm-action Any luck fixing those? pm-utils.x86_64: W: log-files-without-logrotate /var/log/pm-suspend.log Shouldn't you add a Requires: logrotate and add a logrotate file for this? pm-utils.x86_64: W: dangerous-command-in-%pre mv pm-utils.x86_64: W: dangerous-command-in-%post mv Not sure how to get around those, unless you don't need the mv commands anymore. pm-utils-devel.x86_64: W: no-documentation ignore. On the requires, seems like kbd might need to be required for chvt? pciutils was added for a pci.h header, but I don't think it's needed anymore. see bug: 182566 So, I would leave kbd, but remove pciutils...
OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. OK - Spec has consistant macro usage. OK - Meets Packaging Guidelines. OK - License (GPLv2) OK - License field in spec matches OK - License file included in package OK - Spec in American English OK - Spec is legible. OK - Sources match upstream md5sum: f69db402e1869321cac72ffd2f77fa99 pm-utils-1.2.5.tar.gz f69db402e1869321cac72ffd2f77fa99 pm-utils-1.2.5.tar.gz.orig OK - BuildRequires correct OK - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. OK - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime. OK - Package has rm -rf RPM_BUILD_ROOT at top of %install OK - .pc files in -devel subpackage/requires pkgconfig OK - -devel package Requires: %{name} = %{version}-%{release} OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. OK - Package obey's FHS standard (except for 2 exceptions) See below - No rpmlint output. OK - final provides and requires are sane. SHOULD Items: OK - Should build in mock. OK - Should build on all supported archs OK - Should function as described. OK - Should have sane scriptlets. OK - Should have subpackages require base package with fully versioned depend. OK - Should have dist tag OK - Should package latest version OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin OK - check for outstanding bugs on package (merge reviews/rename/re-reviews). Issues: 1. rpmlint says: pm-utils.src: W: strange-permission pm-utils-bugreport-info.sh 0775 pm-utils.src: W: strange-permission pm-utils-99hd-apm-restore 0775 I think we can ignore those. pm-utils.x86_64: W: log-files-without-logrotate /var/log/pm-suspend.log Can be ignored per comment in the spec. pm-utils-devel.x86_64: W: no-documentation Can be ignored. 2. Some non blocking suggestions: Might add a '-p' to your install lines to preserve timestamps of the sources? Currently it's pointless to add smp_mflags, but if there are ever more source files to compile it might be worth considering. I see no blockers at all, so this package is APPROVED. Sorry for the long delay here.