Bug 246748 - Review Request: ohm - open hardware manager (as to be used on OLPC)
Review Request: ohm - open hardware manager (as to be used on OLPC)
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-04 11:40 EDT by Richard Hughes
Modified: 2007-11-30 17:12 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-02 10:52:45 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mclasen: fedora‑review+
kevin: fedora‑cvs+


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

  None (edit)
Description Richard Hughes 2007-07-04 11:40:53 EDT
Spec URL: http://people.freedesktop.org/~hughsient/temp/ohm.spec
SRPM URL: http://people.freedesktop.org/~hughsient/temp/ohm-0.1.0-0.fc7.hughsie.src.rpm

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 http://ohm.freedestop.org 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 14:15:10 EDT
- 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 libohm.so.* shared libs or modules ? if they are dlopened, they
  should better live in /usr/lib/ohm
 
Comment 2 Richard Hughes 2007-07-05 15:58:44 EDT
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/

Richard.
Comment 3 Matthias Clasen 2007-07-05 16:29:05 EDT
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 16:33:42 EDT
one more:

BuildRequires: hal-devel
Comment 5 Matthias Clasen 2007-07-05 17:25:10 EDT
Ok, with these, I finally got it to build in mock. 

Initial impression from the built packages:

/usr/share/ohm-0.1.1/index.html

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 17:33:54 EDT
Translations need to be handled with %find_lang
Comment 7 Richard Hughes 2007-07-05 19:30:39 EDT
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 14:38:21 EDT
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

Add 

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

Otherwise mock complains about an unpackaged file.
Comment 9 Richard Hughes 2007-07-07 06:51:03 EDT
What about the following? Comments?

### BEGIN INIT INFO
# 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.
### END INIT INFO
Comment 10 Matthias Clasen 2007-07-07 10:20:40 EDT
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-09 20:41:03 EDT
How about http://people.freedesktop.org/~hughsient/temp/ohm-0.1.1-0.fc7.src.rpm

Should be okay, and seems to build in mock now.
Comment 12 Matthias Clasen 2007-07-11 16:32:46 EDT
rpmlint output:

[mclasen@localhost Desktop]$ rpmlint
/var/lib/mock/fedora-development-i386/result/ohm-0.1.1-0.fc8.i386.rpm 
W: ohm service-default-enabled /etc/rc.d/init.d/ohmd
[mclasen@localhost Desktop]$ rpmlint
/var/lib/mock/fedora-development-i386/result/ohm-devel-0.1.1-0.fc8.i386.rpm 
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 16:43:34 EDT
Cool, thanks for that. I've fixed all the issues you mentioned.

New SRPM here: http://people.freedesktop.org/~hughsient/temp/ohm-0.1.1-0.fc7.src.rpm

Comment 14 Matthias Clasen 2007-07-11 16:47:39 EDT
Ok, approved.
Comment 15 Richard Hughes 2007-07-12 12:49:45 EDT
Package Name: ohm
Short Description: Open Hardware Manager
Owners: richard@hughsie.com,johnp@redhat.com,mpg@redhat.com
Branches: F-7 OLPC-2
InitialCC: richard@hughsie.com,johnp@redhat.com,mpg@redhat.com
Comment 16 Kevin Fenzi 2007-07-12 13:23:19 EDT
cvs done.

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