Bug 246748

Summary: Review Request: ohm - open hardware manager (as to be used on OLPC)
Product: [Fedora] Fedora Reporter: Richard Hughes <rhughes>
Component: Package ReviewAssignee: Matthias Clasen <mclasen>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: blizzard, fedora-package-review, harald, mclasen, mpg, notting, panemade, richard
Target Milestone: ---Flags: mclasen: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-10-02 14:52:45 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Attachments:
Description Flags
new spec file
none
new spec file none

Description Richard Hughes 2007-07-04 15:40:53 UTC
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 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 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 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/

Richard.

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:

/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 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

Add 

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?

### 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 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 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 20:32:46 UTC
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 20:43:34 UTC
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 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
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 17:23:19 UTC
cvs done.