Partial/initial review comments moved from bug 466343:

1) BuildRequires: tcl-devel would result in some TCL support built in.  Not
sure if it's desirable though, but would be good to explicitly enable/disable
it for reproducible builds.

2) A private copy of libedit is used; would be good to make use of the
separately packaged system libedit.

3) A GUI is included in the sources, but not installed.  Is this on purpose?

4) ipmitool seems to be capable of using freeipmi as well.  I haven't tried it
but I suppose "BuildRequires: freeipmi-devel" would build it in.  Same thing as
comment 1) about reproducible builds applies, but wouldn't it make sense to
split ipmitool into a completely separate package instead of bundling it in
here?

The Mandriva openipmi package seems to have a bunch of improvements over the
Fedora/EL one, such as TCL support, UI (whose separation allows fewer
dependencies in the main package, e.g. TCL) and GUI subpackages.

(In reply to comment #1)
> 4) ipmitool seems to be capable of using freeipmi as well.  I haven't tried it
> but I suppose "BuildRequires: freeipmi-devel" would build it in.  Same thing as
> comment 1) about reproducible builds applies, but wouldn't it make sense to
> split ipmitool into a completely separate package instead of bundling it in
> here?

I've created separate package for ipmitool, see bug #466762. It would be better to review it first and I'll create new .spec for OpenIPMI (without ipmitool and maybe with the ideas mentioned above).

I fixed the most obvious problems in the .spec file and created -gui subpackage with the openipmigui tool. Regarding the problems reported in comment #1:

1) tcl-devel would create binding to TCL language. I think there is no interest in OpenIPMI-tcl subpackage. I explicitly added '--with-tcl=no' option to %configure.

2) yes, there is private libedit library. It's patched version of the upstream one. I admit I don't know why it's there and why it's patched.

3) OpenIPMI-gui subpackage added

4) ipmitool now has separate package

Regarding the Mandriva's OpenIPMI-ui subpackage, IMHO it's roughly equivalent to our OpenIPMI.rpm, while Mandriva's OpenIPMI.rpm ships only libraries (=~ our OpenIPMI-libs.rpm).

Please resume the review with OpenIPMI-2.0.14-4.fc10, available in tomorrow's rawhide or at http://kojipkgs.fedoraproject.org/packages/OpenIPMI/2.0.14/4.fc10/

Created attachment 321170 [details]
Fix for issues 1-4

Ok, continuing review:

1) The use of private libedit deserves a TODO comment in the specfile if modifying the package to use system libedit is not feasible at this point.

2) Parallel make is not used - for a reason it seems (fails in some python build phase), this is also a good thing to document in the specfile.

3) There are some typos in various summaries and descriptions.

4) .cvsignore and sources in CVS still contain the ipmitool tarball.

5) openipmigui does not start (F-9 x86_64, non-IPMI system):

$ openipmigui
Traceback (most recent call last):
  File "/usr/bin/openipmigui", line 310, in <module>
    run(sys.argv)
  File "/usr/bin/openipmigui", line 294, in run
    ui = gui.IPMIGUI(top, mainhandler)
  File "/usr/lib64/python2.5/site-packages/openipmigui/gui.py", line 209, in __init__
    self.tree = Tix.Tree(objpane, options="hlist.columns 2")
  File "/usr/lib64/python2.5/lib-tk/Tix.py", line 1519, in __init__
    ['options'], cnf, kw)
  File "/usr/lib64/python2.5/lib-tk/Tix.py", line 307, in __init__
    self.tk.call(widgetName, self._w, *extra)
_tkinter.TclError: unknown color name "{#d9d9d9}"

6) openipmigui should have a .desktop file.

7) Can openipmigui be usefully be run as non-root?  I'm unable to test because of 5) above.  If not, should be changed to use consolehelper(-gtk).

Fix for issues 1-4 above is attached (I could commit it myself, but CVS ACLs prevent me).

8) %files python and gui should use %{python_sitearch} instead of %{_libdir}/python*/...

1-4) patch applied, thanks
5) the gui does not start because of bug #467224
6) done
7) yes, it can run as regular user, e.g. to manage remote box over IP
8) fixed

Look for OpenIPMI-2.0.14-5.fc10, available at http://koji.fedoraproject.org/koji/buildinfo?buildID=67224 or in rawhide.

Final few findings:

1) *.a should be dropped from -devel or otherwise treated as guidelines suggest:
http://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries


2) $ desktop-file-validate openipmigui.desktop
openipmigui.desktop: error: line "[Desktop Entry] " ends with a space, but looks like a group. The validation will continue, with the trailing spaces ignored.
openipmigui.desktop: warning: key "Encoding" in group "Desktop Entry" is deprecated
openipmigui.desktop: error: value "Application;System" for string list key "Categories" in group "Desktop Entry" does not have a semicolon (';') as trailing character

After fixing these, there's one more:

$ desktop-file-validate openipmigui.desktop
openipmigui.desktop: warning: value "Application;System;" for key "Categories" in group "Desktop Entry" contains a deprecated value "Application"

1) fixed, I compile with --disable-static now. I wonder why the .a were packaged - let's hope it won't break anything important.

2) fixed, desktop-file-validate does not report any problem now.

New build is at http://koji.fedoraproject.org/koji/buildinfo?buildID=68082

Ok, looks good to me now, approved.

Great, thanks a lot!