SSO (single sign-on) is being reconfigured. Maintenance started Jul 16 2:20PM UTC and will last 1 hour. Password-enabled login should still work.
Bug 246748 - Review Request: ohm - open hardware manager (as to be used on OLPC)
Summary: Review Request: ohm - open hardware manager (as to be used on OLPC)
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2007-07-04 15:40 UTC by Richard Hughes
Modified: 2007-11-30 22:12 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2007-10-02 14:52:45 UTC
Type: ---
mclasen: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)
new spec file (2.52 KB, text/plain)
2007-07-05 19:58 UTC, Richard Hughes
no flags Details
new spec file (2.59 KB, text/plain)
2007-07-05 23:30 UTC, Richard Hughes
no flags Details

Description Richard Hughes 2007-07-04 15:40:53 UTC
Spec URL:

Description: OHM is a hardware manager to enforce system policy on embedded devices. It's planed to be used in OLPC for power management. Checkout for more details.

The spec probably needs work, and I'm open to ideas about how to deal with the dlopen'd plugins. Advice and experience welcome. Thanks!

Comment 1 Matthias Clasen 2007-07-05 18:15:10 UTC
- Please use a full source url

- You probably need some requires(post) (pre), etc for
  ldconfig, useradd, etc

- I don't think it is a good idea to use / as the homedir, the wiki says: 
    should usually be a directory created and owned by the package, with 
    appropriately restrictive permissions. One good choice for the location of the 
    directory is the package's data directory in case it has one.

- have you requested an official user id for ohm ?

- please don't mix %{buildroot} and $RPM_BUILD_ROOT in one .spec file

- %dir %{_sysconfdir}/dbus-1/system.d
  should certainly be owned by dbus, no ?

- are* shared libs or modules ? if they are dlopened, they
  should better live in /usr/lib/ohm

Comment 2 Richard Hughes 2007-07-05 19:58:44 UTC
Created attachment 158616 [details]
new spec file

Attached is a new spec file with all the changes.

libohm_*.so.* are dlopened modules so I've made then un-versioned and put them
in /usr/lib/ohm/


Comment 3 Matthias Clasen 2007-07-05 20:29:05 UTC
It misses at least

BuildRequires: perl(XML::Parser)
BuildRequires: dbus-glib-devel 

Still trying to get it to build in mock

Comment 4 Matthias Clasen 2007-07-05 20:33:42 UTC
one more:

BuildRequires: hal-devel

Comment 5 Matthias Clasen 2007-07-05 21:25:10 UTC
Ok, with these, I finally got it to build in mock. 

Initial impression from the built packages:


This looks unusual. Typically, datadirs are not versioned, and
if index.html is documentation, it should probably go into
/usr/share/doc/ohm-0.1.1 instead. The way to do that is to add

%doc index.html 

to the file list

Comment 6 Matthias Clasen 2007-07-05 21:33:54 UTC
Translations need to be handled with %find_lang

Comment 7 Richard Hughes 2007-07-05 23:30:39 UTC
Created attachment 158629 [details]
new spec file

Here you go, this should be everything okay. Cheers for the quick review.

Comment 8 Matthias Clasen 2007-07-06 18:38:21 UTC
rpmlint warns about

W: ohm incoherent-version-in-changelog 0.1.1-0.fc7.hughsie 0.1.1-0.fc8

and it might be a nice idea to add the LSB header to the initscript, to make
Harald feel warm and fuzzy...

Apart from that, I had to do a few more specfile tweaks to get it to build in mock:

Add a 

BuildRequires: libtool


rm -f %{buildroot}%{_datadir}/ohm-%{version}/index.html

Otherwise mock complains about an unpackaged file.

Comment 9 Richard Hughes 2007-07-07 10:51:03 UTC
What about the following? Comments?

# Provides:          ohmd
# Required-Start:    haldaemon
# Required-Stop:     
# Should-Start:	     
# Should-Stop:	     
# Default-Start:     2 3 4 5
# Default-Stop:      1 6
# Short-Description: Loads OHM Open Hardware Manager
# Description:       Loads Open Hardware Manager to enforce power policy on
#                    embedded and small form factor devices.

Comment 10 Matthias Clasen 2007-07-07 14:20:40 UTC
Looks fine to me. You can omit the empty fields. And I think you probably want to
add 0 to Default-Stop, not sure if it makes any difference.

Comment 11 Richard Hughes 2007-07-10 00:41:03 UTC
How about

Should be okay, and seems to build in mock now.

Comment 12 Matthias Clasen 2007-07-11 20:32:46 UTC
rpmlint output:

[mclasen@localhost Desktop]$ rpmlint
W: ohm service-default-enabled /etc/rc.d/init.d/ohmd
[mclasen@localhost Desktop]$ rpmlint
W: ohm-devel no-documentation

both are ignorable, imo

package name: ok
spec file name: ok
packaging guidelines: ok
license: ok
license file: ok
license file included: ok
spec file language: ok
spec file readability: excellent
upstream sources: ok
buildable: ok
BRs: ok
locale handling: ok
ldconfig: ok
relocatable: n/a
directory ownership: BAD, -devel must require pkgconfig for /usr/lib/pkgconfig
file list dupes: ok
defattr: ok
%clean: ok
macro use: ok
permissible content: ok
doc package: n/a
%doc: ok
headers: ok
static libs: n/a
.pc files:  BAD, see above
shared libs: ok
-devel requires base: ok
la files: ok
desktop file: n/a
directory ownership: ok
%install: BAD, must do rm -rf %{buildroot} at the beginning of %install
utf8 filenames: ok

Two mustfix items, and one whishlist item, then you are good to go:

- Must require pkgconfig in -devel
- Must clean buildroot in %install
- Should include AUTHORS and README in %doc  

Comment 13 Richard Hughes 2007-07-11 20:43:34 UTC
Cool, thanks for that. I've fixed all the issues you mentioned.

New SRPM here:

Comment 14 Matthias Clasen 2007-07-11 20:47:39 UTC
Ok, approved.

Comment 15 Richard Hughes 2007-07-12 16:49:45 UTC
Package Name: ohm
Short Description: Open Hardware Manager
Branches: F-7 OLPC-2

Comment 16 Kevin Fenzi 2007-07-12 17:23:19 UTC
cvs done.

Note You need to log in before you can comment on or make changes to this bug.