Bug 226212 - Merge Review: OpenIPMI
Summary: Merge Review: OpenIPMI
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Ville Skyttä
QA Contact: Fedora Package Reviews List
Depends On:
TreeView+ depends on / blocked
Reported: 2007-01-31 20:18 UTC by Nobody's working on this, feel free to take it
Modified: 2008-10-31 08:14 UTC (History)
2 users (show)

Fixed In Version: 2.0.14-6
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-10-30 22:22:27 UTC
Type: ---
ville.skytta: fedora-review+

Attachments (Terms of Use)
Fix for issues 1-4 (2.39 KB, patch)
2008-10-22 15:58 UTC, Ville Skyttä
no flags Details | Diff

Description Nobody's working on this, feel free to take it 2007-01-31 20:18:33 UTC
Fedora Merge Review: OpenIPMI

Initial Owner: pknirsch@redhat.com

Comment 1 Ville Skyttä 2008-10-12 19:48:20 UTC
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

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.

Comment 2 Jan Safranek 2008-10-13 14:13:47 UTC
(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).

Comment 3 Jan Safranek 2008-10-20 12:10:18 UTC
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/

Comment 4 Ville Skyttä 2008-10-22 15:58:14 UTC
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>
  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).

Comment 5 Ville Skyttä 2008-10-22 16:04:10 UTC
8) %files python and gui should use %{python_sitearch} instead of %{_libdir}/python*/...

Comment 6 Jan Safranek 2008-10-23 12:57:51 UTC
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.

Comment 7 Ville Skyttä 2008-10-26 17:16:46 UTC
Final few findings:

1) *.a should be dropped from -devel or otherwise treated as guidelines suggest:

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"

Comment 8 Jan Safranek 2008-10-30 08:53:32 UTC
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

Comment 9 Ville Skyttä 2008-10-30 22:22:27 UTC
Ok, looks good to me now, approved.

Comment 10 Jan Safranek 2008-10-31 08:14:09 UTC
Great, thanks a lot!

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