Bug 507089

i've attempted to apply all the lessons i learned during the olpc-kbdshim review.  hope i've succeeded.

i'm aware that the post-install hook that disables ohmd is almost certainly not acceptable.  ohmd and olpc-powerd cannot run at the same time, though they can co-exist otherwise.  i'm hoping for advice on this.

i also just noticed that the Source0: line is incorrect -- my automated package build commmands aren't correct yet.  my apologies.

(In reply to comment #1)
> i've attempted to apply all the lessons i learned during the olpc-kbdshim
> review.  hope i've succeeded.

We'll see. ;) Stay tuned.

Sorry it took so long, but the scriptlets really caused me some headache...

(In reply to comment #1)

> i'm aware that the post-install hook that disables ohmd is almost certainly not
> acceptable.  ohmd and olpc-powerd cannot run at the same time, though they can
> co-exist otherwise.  

It's definitely not according to the guidelines, see
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Why_don.27t_we....

But as ohmd is olpc specific and not in Fedora IMO we can make an exception here.


REVIEW FOR 55f351e35ee8dbb622c4565a8a7b1412  olpc-powerd-7-1.src.rpm


FIX - MUST: rpmlint must be run on every package. The output should be posted in the review:
$ rpmlint Desktop/olpc-powerd-*
olpc-powerd.i586: W: incoherent-version-in-changelog 6-2 ['7-1.fc11', '7-1']
  => See https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
olpc-powerd.i586: W: devel-file-in-non-devel-package /usr/bin/powerd-config
  => false positive, see https://bugzilla.redhat.com/show_bug.cgi?id=468987#c1
olpc-powerd.i586: W: dangerous-command-in-%preun rm
  => that's /var/run/powerevents, save to igonore.

OK - MUST: The package is named according to the Package Naming Guidelines.
OK - MUST: The spec file name matches the base package %{name}, in the format %{name}.spec.
OK - MUST: The package meets the Packaging Guidelines.
OK - MUST: The package is licensed with a Fedora approved license and meets the Licensing Guidelines: GPLv2+
OK - MUST: The License field in the package spec file matches the actual license.
FIX - MUST: The license file from the source package is included in %doc, but it's LGPLv2.1
OK - MUST: The spec file is in American English.
OK - MUST: The spec file for the package is legible.
N/A - MUST: The sources used to build the package match the upstream source by MD5 (Git checkout)
OK - MUST: The package successfully compiles and builds into binary rpms on %{ix86}
N/A - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch.
OK - MUST: All build dependencies are listed in BuildRequires.
N/A - MUST: The spec file handles locales properly with the %find_lang macro.
N/A - MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
N/A - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package.
OK - MUST: The package owns all directories that it creates.
FIX - MUST: The package contains duplicate files in the %files listing:

warning: File listed twice: /etc/event.d/olpc-switchd
warning: File listed twice: /etc/event.d/powerd
warning: File listed twice: /etc/powerd/pleaseconfirm.pgm
warning: File listed twice: /etc/powerd/powerd.conf
warning: File listed twice: /etc/powerd/shuttingdown.pgm

OK - MUST: Permissions on files are set properly. Every %files section includes a %defattr(...) line.
OK - MUST: The package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
OK - MUST: The package consistently uses macros, as described in the macros section of Packaging Guidelines.
OK - MUST: The package contains code, or permissable content.
N/A - MUST: Large documentation files should go in a -doc subpackage.
OK - MUST: Files included as %doc do not affect the runtime of the application.
N/A - MUST: Header files must be in a -devel package.
N/A - MUST: Static libraries must be in a -static package.
N/A - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'.
N/A - MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
N/A - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release}
OK - MUST: The package does not contain any .la libtool archives.
N/A - MUST: Packages containing GUI applications must include a %{name}.desktop file.
OK - MUST: The packages does not own files or directories already owned by other packages.
OK - MUST: At the beginning of %install, the package runs rm -rf $RPM_BUILD_ROOT.
OK - MUST: All filenames in rpm packages are valid UTF-8.



SHOULD Items:
N/A - SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
N/A - SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
OK - SHOULD: The the package builds in mock.
OK - SHOULD: The package should compile and build into binary rpms on all supported architectures.
OK - SHOULD: The package functions as described.
FIX - SHOULD: If scriptlets are used, those scriptlets must be sane.
N/A - SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
N/A - SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg.
OK - SHOULD: No file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin.


Other items:
OK - latest version packaged
OK - RPM_OPT_FLAGS honored
OK - Timestamps are preserverd


Issues: 

- Scriptlets: 
%preun
initctl stop olpc-switchd
initctl stop powerd
rm -f /var/run/powerevents

The rest could also be done in %postun

if test -e /etc/init.d/ohmd
then
    /etc/init.d/ohmd start
    chkconfig ohmd on
fi


- scriptlets should run silent. Add ||: at the end of the last command, so the scripts will always return 0. If they return 1, rpm will abort the transaction.

- Use conditionals for the scriptlets, see https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Syntax
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscript_packaging


- License: Source says it's GPLv2+, but COPYING is LGPLv2.1

On the scriptlets again...

Your scripts only handle the case where one replaces ohm with powerd, but not when upgrading powerd. The problem is the order: Upon an upgrade, %preun of the old package runs after %post of the new package, see
https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Scriptlet_Ordering

So what we get is:

initctl start powerd
initctl start olpc-switchd
...
initctl stop olpc-switchd
initctl stop powerd

This cannot work! How about this:

# Only on install
%post
if [ $1 = 1 ] ; then
    if test -e /etc/init.d/ohmd ; then
        service ohmd stop >/dev/null 2>&1
        chkconfig ohmd off
    fi
    initctl -q start powerd
    initctl -q start olpc-switchd
fi

# Only on uninstall
%preun
if [ $1 = 0 ] ; then
    initctl stop -q olpc-switchd
    initctl stop -q powerd
    if test -e /etc/init.d/ohmd
    then
        /sbin/service ohmd start >/dev/null 2>&1
        /sbin/chkconfig ohmd on
    fi
fi

# Restart after upgrade
%postun
if [ "$1" -ge "1" ] ; then
    initctl stop -q olpc-switchd
    initctl stop -q powerd
    initctl start -q powerd
    initctl start -q olpc-switchd
fi

One more thing: The rm -rf in %preun is not nice and causes an rpmlint warning. We should let the package own the file, then rpm will remove it:

%install
...
mkdir -p $RPM_BUILD_ROOT%{_localstatedir}/run

...
touch $RPM_BUILD_ROOT%{_localstatedir}/run/powerevents

%files
...
%ghost(missingok) %{_localstatedir}/run/powerevents

IMO this is the best solution, but we don't have a guideline for files in /var/run. On my machine half of the files is owned by a package, the others are not. I think I should ask packaging committee about this and/or write a proposal.

BTW: he scripts from comment #4 result in a couple of additional deps:

Requires(post): chkconfig
Requires(preun): chkconfig
# This is for /sbin/service
Requires(preun): initscripts

See: https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscript_packaging

i believe i've addressed all of the FIX issues listed above.

rpmlint now gives 0 warnings/0 errors -- however, my development machine is currently fedora 9.  i did nothing for the "false positive" error that cwickert reported.  

regarding comment #5, for the "unsafe" rm -- i changed powerd to clean up after itself, so that the rm in the uninstall is no longer needed.

i had the best intentions of testing the install/remove/upgrade paths for the scriptlets this weekend, but was not able to.  i agree, given my reading of the spec, with cwickert's proposed implementation in comment #4, and have applied it verbatim.

so:  here are the new pre-release srpm and spec:
 http://dev.laptop.org/~pgf/rpms/srpms/olpc-powerd-8-1.20090720git702fd8e.src.rpm
 http://dev.laptop.org/~pgf/rpms/srpms/olpc-powerd.spec-8-1.20090720git702fd8e

and an f-9 rpm:
 http://dev.laptop.org/~pgf/rpms/olpc-powerd-8-1.20090720git702fd8e.fc9.i386.rpm

i will test at earliest opportunity, but if someone else tests sooner, please let me know.

many thanks.

One last thing I have spotted: We also need to require upstart for initctl:
# these are all for the manipulation of powerd
Requires(post): upstart
Requires(postun): upstart
Requires(preun): upstart


I have tested the package on my XO 1 powerd works fine, keys work etc.

test case for scriptlets:
$ /etc/init.d/ohmd status
$ rpm -Ivvh olpc-powerd-8-1.20090720git702fd8e.fc9.i386.rpm
$ /etc/init.d/ohmd status
$ ps -A | grep powerd

Upgrade:
$ rpm -Uvvh olpc-powerd-8-1.20090720git702fd8e.fc9.i386.rpm --force
$ ps -A | grep powerd
 (other process id)
$ /etc/init.d/ohmd status

Removal:
$ rpm -evv olpc-powerd
$ ps -A | grep powerd
 (other process id)
$ /etc/init.d/ohmd status

-> works as it should.

The package is APPROVED

... but please don't forget to add the upstart requires after import.

(In reply to comment #7)
> rpmlint now gives 0 warnings/0 errors -- however, my development machine is
> currently fedora 9.  i did nothing for the "false positive" error that cwickert reported.  

I still get warnings for the binary package:

$ rpmlint olpc-powerd-8-1.20090720git702fd8e.fc9.i386.rpm 
olpc-powerd.i386: W: incoherent-version-in-changelog 8-1 ['8-1.20090720git702fd8e.fc9', '8-1.20090720git702fd8e']
olpc-powerd.i386: W: devel-file-in-non-devel-package /usr/bin/powerd-config

The latter is bogus, the first is rpmlint not accepting the your changelog style. However according to https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs it is valid.


I have overlooked something, an issue from comment # 3 is still open:
- scriptlets should run silent. Add ||: at the end of the last command, so the
scripts will always return 0. If they return 1, rpm will abort the transaction.

i need to set up mock on f11 in order to recreate the rpmlint warnings.

as for the silent scripts, the guidelines implied (and my shell experience says) that "exit 0" would work for this.  i guess you're saying it does not?

(again, apologies -- i have not had a chance to do an actual test with the new packages yet.)

Uhh, I have somehow overseen the "exit 0". Need to get some sleep.

i will add the dependency on upstart.

many thanks, again.

actually, i'm a little bit confused by the need to add the upstart dependency.

powerd itself requires the presence of upstart in order to run.  why must the dependency be called out separately for the scriptlets?  is it even possible to configure a modern fedora without upstart?

Practically not, theoretically yes. Do as you like.

New Package CVS Request
=======================
Package Name: olpc-powerd
Short Description: power management for the XO laptop
Owners: pgf
Branches: F-11
InitialCC: cwickert

CVS done.

olpc-powerd-9-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/olpc-powerd-9-1.fc11

olpc-powerd-9-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.