Bug 226302

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.