Bug 226212
Summary: | Merge Review: OpenIPMI | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nobody's working on this, feel free to take it <nobody> | ||||
Component: | Package Review | Assignee: | Ville Skyttä <ville.skytta> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | jsafrane, pknirsch | ||||
Target Milestone: | --- | Flags: | ville.skytta:
fedora-review+
|
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | 2.0.14-6 | Doc Type: | Bug Fix | ||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2008-10-30 22:22:27 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Attachments: |
|
Description
Nobody's working on this, feel free to take it
2007-01-31 20:18:33 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 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! |