Bug 469931 - Review Request: ipmiutil - IPMI Management Utilities
Review Request: ipmiutil - IPMI Management Utilities
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
http://ipmiutil.sourceforge.net
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-04 14:46 EST by Andy Cress
Modified: 2014-11-26 07:29 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-11-05 08:49:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review?
limburgher: fedora‑cvs+


Attachments (Terms of Use)
A comparison of common IPMI open source packages (37.15 KB, text/html)
2008-11-05 10:52 EST, Andy Cress
no flags Details

  None (edit)
Description Andy Cress 2008-11-04 14:46:49 EST
Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.2.4-1.src.rpm
Description: ipmiutil provides various utilities to manage an IPMI system, such as view the IPMI firmware log, perform an IPMI reset, configure the IPMI LAN interface, get and set sensor thresholds, etc.  It supports various drivers or a driverless mode, useful for bootables.  Ipmiutil was formerly known as 'panicsel'.
Comment 1 Bill Nottingham 2008-11-04 14:56:30 EST
How does this differ in general from ipmitool? There seems to be quite a bit of overlap here.
Comment 2 Andy Cress 2008-11-05 10:52:50 EST
Created attachment 322599 [details]
A comparison of common IPMI open source packages

A good question that I have been asked before.  There is significant overlap between the two, with the key difference being the architectural approach:  ipmiutil is more top-down with the focus being to optimize common management tasks, while ipmitool is more bottom-up with the focus being to expose granular IPMI features.  ipmiutil was started in 2001, while ipmitool was started in 2003.  Attached is my assessment of a feature comparison, including the relative advantages of each.  There are precedents for making more than one choice available in the same space, and given that ipmiutil currently has a significant installed base, it makes sense to include it in Fedora, since SLES, MontaVista, and RedFlag have also included it.  
ipmiutil project stats, avg over last 60 days: (as of 11/04/08)
    325   Hits/day
    134   Pages/day
     28.3 MB/day
      9.4 Downloads/day
Comment 3 Dan Horák 2008-12-08 08:54:48 EST
Hi Andy,
I will do the review, but the recent spec needs a lot of work to be acceptable for Fedora. Please get comfortable with https://fedoraproject.org/wiki/Packaging/Guidelines and other docs at https://fedoraproject.org/wiki/PackageMaintainers#Packaging

- the Source tag has wrong format - https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net
- "%ifarch x86_pentium3 x86_pentium4" is useless on Fedora
- the %ifarch to %define LibDir - use only %{_libdir} in the spec
- do not use absolute paths, use macros %{_{bin,sbin,lib,...}dir}
- do not check whether %{buildroot} == "/" in %install and %clean
- do not gzip man pages, it is done automagically
- drop the %pre and %post scripts almost completely, rely on the content that Fedora provides (we have net-snmp, specific location for MIBs, ...), they should contain handling of the install shared library (call ldconfig) and take care of initscript
- use only the new names for utils (i_*) to prevent conflicts with other packages
Comment 4 Andy Cress 2008-12-10 15:04:34 EST
Dan,
Thanks for the input.  I've started on these items, and have committed the changes so far into the project svn.
(http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec?view=markup)

- the Source tag has wrong format 
A: fixed in the ipmiutil.spec in svn
- "%ifarch x86_pentium3 x86_pentium4" is useless on Fedora 
A: yes, inert for Fedora, but required for the MontaVista distro
- the %ifarch to %define LibDir - use only %{_libdir} in the spec 
A: fixed in ipmiutil.spec
- do not use absolute paths, use macros %{_{bin,sbin,lib,...}dir} 
A: changed in ipmiutil.spec
- do not check whether %{buildroot} == "/" in %install and %clean 
A: fixed in ipmiutil.spec
- do not gzip man pages, it is done automagically  
A: does this mean that the installed files should be *.8?  What process does this (at rpm install time, perhaps)?
- drop the %pre and %post scripts almost completely, rely on the content that
Fedora provides (we have net-snmp, specific location for MIBs, ...), they
should contain handling of the install shared library (call ldconfig) and take
care of initscript 
A: significant changes, and removing some stuff.  How should I detect the MIB directory, if present?  
- use only the new names for utils (i_*) to prevent conflicts with other
packages 
A: I removed the symlinks for commands, there are back-compatibility issues here, this may need further investigation.

The rpmlint output with the updated spec file:
# rpmlint ipmiutil-2.3.2-1.i386.rpm
E: ipmiutil arch-dependent-file-in-usr-share /usr/share/ipmiutil/ipmi_port
E: ipmiutil arch-dependent-file-in-usr-share /usr/share/ipmiutil/events
W: ipmiutil dangerous-command-in-%post ln
W: ipmiutil dangerous-command-in-%postun rm
#

So where should extra ipmiutil-specific binaries go?  They do not need to be in the PATH, IMO.  I tried several locations, but rpmlint wasn't happy with my choices.  
The last warning seems frivilous to me, it seems removing extra stuff should be allowed in %postun.
Comment 5 Dan Horák 2008-12-17 08:16:56 EST
Please prepare a new srpm and spec file for download so I can continue in the review (with updated Version and/or Release).

Fedora prefers small clean spec files without support for other distros, because the use of miscellaneous conditions lowers the legibility and can lead to errors.

The present usage of pre/post scripts is wrong, the MIBs and cron files all have their proper (and stable) place in the system and their installation and removing is done by standard means of the rpm program, it can even react on user modified files (e.g. save copies). Some locations are standardized in LSB or FHS, so they are even stable within multiple distros.

All binaries that are supposed to be run directly by a (super-)user must live in "bin" or "sbin". Test or demo scripts not required for proper function of a package can be marked and stored as %doc.
Comment 6 Andrew Cress 2008-12-19 08:44:21 EST
Dan,

SPEC URL: http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec?view=markup
SRC RPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.3.2-1.src.rpm

RE Fedora-only spec files:  I think there are lots of counter-examples of projects in Fedora that have support for other distros (net-snmp for one), which certainly makes sense for the project maintainers.  Certainly we both want to make the spec file as simple as possible.

I could move the MIB and cron files from %post into the %files section, and that would probably work for the init.d scripts as well.  

I'll move the two binaries in question to /usr/sbin like the others, but I'll have to change 'events' to 'ievents'.  And if I change some command names, this will have an impact on some legacy users, so the version will have to bump to 2.4.0 or 3.0.0 when these changes are made.
Comment 7 Prarit Bhargava 2009-02-18 06:32:46 EST
(In reply to comment #6)
> Dan,
> 
> SPEC URL:
> http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec?view=markup
> SRC RPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.3.2-1.src.rpm
> 
> RE Fedora-only spec files:  I think there are lots of counter-examples of
> projects in Fedora that have support for other distros (net-snmp for one),
> which certainly makes sense for the project maintainers.  Certainly we both
> want to make the spec file as simple as possible.
> 
> I could move the MIB and cron files from %post into the %files section, and
> that would probably work for the init.d scripts as well.  
> 
> I'll move the two binaries in question to /usr/sbin like the others, but I'll
> have to change 'events' to 'ievents'.  And if I change some command names, this
> will have an impact on some legacy users, so the version will have to bump to
> 2.4.0 or 3.0.0 when these changes are made.

Hey Andrew, just wondering where things are with this?  If you need any help please let me know.  I'd like to see this get into F11 (which will be the base for RHEL6!).

P.
Comment 8 Dan Horák 2009-02-18 06:52:25 EST
It is stuck on me :-( I have few ideas, but there is still a lot of work before it can be accepted into Fedora.
Comment 9 Andy Cress 2009-06-25 14:28:10 EDT
ipmiutil-2.4.0 is now released with some updates for Fedora issues.

SPEC URL:
http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec
SRC RPM URL:
http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.4.0-1.src.rpm

I moved the ipmi_port and events binaries to /usr/sbin, and changed 'events' to 'ievents'.
I moved the init.d and cron scripts from the data directory to go directly into /etc/rc.d/init.d and /etc/cron.daily via the %files rather than copying them in %post.
Comment 10 Andy Cress 2009-07-21 12:24:18 EDT
Dan,
Any feedback on the latest SPEC & SRC RPM from ipmiutil-2.4.0?
I think this should be much better wrt compliance, perhaps sufficient.  
Andy
Comment 11 Dan Horák 2009-08-17 03:28:55 EDT
Andy, I am sorry, but I must step down as the reviewer. I am very busy with other work and can't dedicate the required amount of time for this review.
Comment 12 Jason Tibbitts 2009-08-20 18:10:19 EDT
I took a look at the package from comment #9.  Unfortunately it failed to build in rawhide:

if gcc -DHAVE_CONFIG_H -I. -I. -I../.. -I. -I./inc     -g -O2 -MT lanplus.o -MD -MP -MF ".deps/lanplus.Tpo" -c -o lanplus.o lanplus.c; \
        then mv -f ".deps/lanplus.Tpo" ".deps/lanplus.Po"; else rm -f ".deps/lanplus.Tpo"; exit 1; fi
lanplus.c:70:26: error: openssl/rand.h: No such file or directory
make[3]: *** [lanplus.o] Error 1

Perhaps a missing build dependency?  A scratch build with a full log is at http://koji.fedoraproject.org/koji/taskinfo?taskID=1618755
Comment 13 Andy Cress 2009-08-21 10:44:09 EDT
It does depend upon openssl-devel, and that should be checked.  I'll add that.
Comment 14 Andy Cress 2009-09-14 14:20:50 EDT
The BuildRequires: openssl-devel was added in ipmiutil-2.4.2 and later.
The current release is now ipmiutil-2.4.3, see 
SPEC URL:
http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec
SRC RPM URL:
http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.4.3-1.src.rpm
Comment 15 Jason Tibbitts 2009-11-07 21:57:49 EST
The package in comment 14 fails to build for me:

+ cd /builddir/build/BUILDROOT/ipmiutil-2.4.3-1.x86_64/usr/share/man/man8
+ gzip -f '*.8'
gzip: *.8: No such file or directory

I'm not sure what's gone wrong, but do note that you shouldn't try to compress the manpages yourself; rpmbuild will do it automatically.

The %ifarch doesn't seem at all relevant to Fedora, and I have to admit that I can't understand why you would define all of those macros that are pretty much the same as existing macros or are longer than the strings they replace.

I don't understand the purpose of %pre; packages cannot produce output in scriptlets, so the whole thing seems to be pointless.  And the checks for SuSE and Montavista in %post have no place in Fedora.  Many other portions of the scriptlets don't seem relevant to Fedora.

Please clear the whiteboard when you have a package which builds.

I checked the account system and it looks like you are not in the packager group, so I've indicated that you need a sponsor.  Please read through http://fedoraproject.org/wiki/Join_the_package_collection_maintainers for more information on becoming a Fedora packager.
Comment 16 Andy Cress 2009-11-20 15:54:44 EST
The gzip was an artifact from older version that wasn't needed, but hadn't caused problems before.   I've taken it out now, and cleaned up more stuff in the spec file.   I have moved any distro-specific logic from the spec file and handled it via configure.in.  Those changes are now committed to SVN, the updated spec file is at 
http://ipmiutil.svn.sourceforge.net/viewvc/ipmiutil/trunk/doc/ipmiutil.spec
and this will be included in ipmiutil-2.5.1 soon.  

I have also applied to be a Fedora packager (as user arcress).
Comment 17 Andy Cress 2009-11-20 16:46:47 EST
The new src.rpm is now at
http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.5.1-1.src.rpm
Comment 18 Jason Tibbitts 2010-01-25 18:49:40 EST
This is still marked as not building; you seem to have posted a new package but not cleared the whiteboard.  If you're wondering why nobody's commented on your ticket, that certainly contributes (as it won't appear in the review queue).  I've cleared that for you.

You may still want to address the existing rpmlint complaints:

  ipmiutil.x86_64: W: incoherent-version-in-changelog
   2.5.1 ['2.5.1-1', '2.5.1-1']
Your changelog entry isn't in the proper format (missing the package release)

  ipmiutil.x86_64: W: dangerous-command-in-%post cp
  ipmiutil.x86_64: W: dangerous-command-in-%postun rm
You should justify these.

  ipmiutil.x86_64: E: use-tmp-in-%post    
You should verify that any usage of /tmp or /var/tmp in your scriptlets is secure.

  ipmiutil.x86_64: W: service-default-enabled /etc/rc.d/init.d/ipmiutil_asy
  ipmiutil.x86_64: W: service-default-enabled /etc/rc.d/init.d/ipmiutil_wdt
Don't enable services by default.

  ipmiutil.x86_64: W: no-reload-entry /etc/rc.d/init.d/ipmiutil_asy
  ipmiutil.x86_64: W: no-reload-entry /etc/rc.d/init.d/ipmiutil_wdt
Service scripts should have the usual set of targets.

  ipmiutil.x86_64: E: subsys-not-used /etc/rc.d/init.d/ipmiutil_asy
  ipmiutil.x86_64: E: subsys-not-used /etc/rc.d/init.d/ipmiutil_wdt
No use of /var/lock/subsys for proper lockfiles.

  ipmiutil.x86_64: W: service-default-enabled /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: W: missing-lsb-keyword Required-Stop in /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: W: missing-lsb-keyword Short-Description in /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: W: service-default-enabled /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: E: no-status-entry /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: W: no-reload-entry /etc/rc.d/init.d/ipmi_port
  ipmiutil.x86_64: E: subsys-not-used /etc/rc.d/init.d/ipmi_port
In the same basic vein as the above.

  ipmiutil-debuginfo.x86_64: E: empty-debuginfo-package
Debuginfo generation is broken.  Verify that the proper compiler flags are used (-g) and that nothing strips the binaries.
Comment 19 Andy Cress 2010-01-28 17:34:18 EST
Jason,

Thank you for recognizing that it was in limbo.
I have updated all of the rpmlint issues in this version, except for the debuginfo.  I'll fix that next.
  http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.5.3-1.src.rpm
Comment 20 Andy Cress 2010-01-29 17:27:24 EST
OK, I found the problem with debuginfo, and that works now also.
Here is a pre-release version of ipmiutil-2.5.4 with that fixed:
   http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.5.4-1.src.rpm 
I believe that this version is clean, but let me know if you find anything.
Comment 21 Andy Cress 2010-02-12 11:44:08 EST
The ipmiutil-2.5.4 release is now finalized, and posted to sourceforge.
See http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.5.4-1.src.rpm 
Please review and comment as to next steps.
Comment 22 Terje Røsten 2010-02-16 12:06:43 EST
Andy, are you the upstream maintainer too?

The rpm might do the job a standalone download, but there are a lot issues to be
fixed before this can be accepted in Fedora.

For starters clean up the scripts, they are a mess. If possible remove them entirely.

Then, please clearly explain what is used to query already IMPI enabled device,
and what parts which turn the host machine into a IMPI enabled device.

Then split the package accordingly. 

Remove all %define, they look very strange and with no purpose.

Many on the commands have to generic names; sensor, alarms, hwreset etc
and will never be accepted. Consider to fold everything in one command with
sub commands

$ ipmiutil sensor
$ impiutil hwreset

etc

There are more (e.g. optflags, changelog), however fix these and I will continue.
Comment 23 Andy Cress 2010-02-16 14:02:42 EST
Yes, I am the maintainer.

OK, the %define no longer have a purpose, that makes sense.

The package can be installed on an IPMI-enabled system, or on a non-IPMI system to be used as a remote client.  the only difference would be that the initial IPMI data would not be gathered on a non-IPMI client.  Detecting whether or not a system is IPMI-enabled (has an IPMI Management Controller) is done by running any ipmiutil command to see if it can find the interface.  No changes are made to the IPMI configuration.  

The software does already fold all functions into one meta-command as you mention, and the other commands had previously been changed to scripts that just invoke the one command method.  They were provided in ipmiutil-2.1.0 as a back-compatible path for previously deployed user scripts.  
But, it is time to wean the users off of the old scripts, and I'll start that for ipmiutil-2.6.0.
Comment 24 Terje Røsten 2010-02-16 15:30:11 EST
Okay, I believe I understand things a bit better now. Please post a updated spec when 2.6 is out and I will have fresh look.
Comment 25 Andy Cress 2010-02-16 15:51:40 EST
I have found that I need some input about this.
I could simply remove the command scripts for hwreset,sensor,etc. with appropriate warning to legacy users that they are now defunct, but renaming the man pages is the issue where I need input.
In renaming the man pages my proposal is to name it to match the meta-command subfunction with a prefix, and using "i" as the prefix makes sense to me, given that we don't want it to be too long to type.  Below is the result.
I need to know if that is sufficiently unique or not before I go through and proliferate the new naming scheme.  
There are previous examples in other projects of this type of prefix, for example the SCSI list utility is named 'slist'.

old name       new name     existing meta-command referred to
----------     ----------   ----------
ipmiutil.8     ipmiutil.8   (ipmiutil)
alarms.8       ialarms.8    (ipmiutil alarms)
bmcconfig.8    iconfig.8    (ipmiutil config)
bmchealth.8    ihealth.8    (ipmiutil health)
fruconfig.8    ifru.8       (ipmiutil fru)
getevent.8     igetevent.8  (ipmiutil getevent)
hwreset.8      ireset.8     (ipmiutil reset)
icmd.8         icmd.8       (ipmiutil cmd)
idiscover.8    idiscover.8  (ipmiutil discover)
ievents.8      ievents.8    (ipmiutil events)
isolconsole.8  isol.8       (ipmiutil sol)
pefconfig.8    ilan.8       (ipmiutil lan)
sensor.8       isensor.8    (ipmiutil sensor)
showsel.8      isel.8       (ipmiutil sel)
tmconfig.8     iserial.8    (ipmiutil serial)
wdt.8          iwdt.8       (ipmiutil wdt)
Comment 26 Terje Røsten 2010-02-16 16:29:23 EST
sounds reasonable to me, the alternative is the git way (git-foo, git-bar ..etc), however git has the advantage of being short itself.
Comment 27 Andy Cress 2010-02-26 18:35:05 EST
The new ipmiutil-2.6.0 release is posted now, with the subcommand renaming included.
See    http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.0-1.src.rpm
Comment 28 Terje Røsten 2010-02-28 16:22:45 EST
Thanks Andy much better, some comments:

> Name: ipmiutil
> Version: 2.6.0
> Release: 1
> Summary: A package that includes various IPMI server management utilities
> License: BSD
> Vendor: Kontron America, Inc.
> Group: Applications/System
> Source: http://downloads.sourceforge.net/%{name}/%{name}-%{version}.tar.gz
> URL: http://ipmiutil.sourceforge.net
> BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root
> BuildRequires: openssl-devel

Move Summary: to the top, remove Vendor: line (it's added by the Fedora build system), add disttag (https://fedoraproject.org/wiki/Packaging:DistTag):

Release:  1%{?dist}

Change BuildRoot: to:

BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

Optional: alignment of values.

> %build
> sh configure
> make

Add  -j flags to make and use configure macro:

%build
%configure
make %{?_smp_mflags}

> %install
> # %{buildroot} is set above in the header so it will never be "/"
> # if [ "%{buildroot}" != "/" ]
> rm -rf %{buildroot}

Remove the comment.

> export RPM_BUILD_DIR=`pwd`
> export RPM_BUILD_ROOT=%{buildroot}
> make install DESTDIR=%{buildroot}
> # AFTER_INSTALL

export stuff looks strange, really needed? Remove comment.

> %clean
> # %{buildroot} is set above in the header so it will never be "/"
> rm -rf %{buildroot}

Move this section to before %files section and remove comment

> %files
> %defattr(0755,root,root)

Change to 

 %defattr(- , root, root, -)

> /etc/cron.daily/checksel

Use sysconfdir for /etc

> %defattr(0664,root,root)

Remove.

> %{_datadir}/%{name}/README
> %{_datadir}/%{name}/COPYING
> %{_datadir}/%{name}/UserGuide

Remove too, add as %doc first in %files 

> %{_mandir}/man8/isel.8.gz
> %{_mandir}/man8/isensor.8.gz

Postfix might change to e.g. xz, change all mans to this style:

%{_mandir}/man8/isensor.8*


> # %defattr(-,root,root)

Remove

> # %doc README TODO COPYING ChangeLog

Uncomment and move to top of %files, change to (add UserGuide here) 

%doc AUTHORS ChangeLog COPYING doc NEWS README TODO
%doc doc/UserGuide

What's left then are the scripts, looks much better, but still
need clean up. A better aproach would be to move the logic to 
init scripts itself. The spec should only have
( https://fedoraproject.org/wiki/Packaging:SysVInitScript ):


Requires(post): chkconfig
Requires(preun): chkconfig
# This is for /sbin/service
Requires(preun): initscripts
...
%post
# This adds the proper /etc/rc*.d links for the script
/sbin/chkconfig --add <script>

%preun
if [ $1 = 0 ] ; then
    /sbin/service <script> stop >/dev/null 2>&1
    /sbin/chkconfig --del <script>
fi


We are making progress, please keep up the good work.

Please include updated links to spec and srpm file when doing changes.
Comment 29 Andy Cress 2010-03-18 17:43:13 EDT
OK, now we have ipmiutil-2.6.1 which should have almost everything conforming.

Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.1-1.src.rpm

I didn't add the -j flags to make, though, since it seemed to occasionally cause some threads to do things in parallel that needed to be in sequence.
Comment 30 Andy Cress 2010-04-27 12:00:41 EDT
I have also released ipmiutil-2.6.3 which has further improvements, and should still be conforming.  Please review this.

Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.3-1.src.rpm
Comment 31 Terje Røsten 2010-04-28 13:56:39 EDT
Hi Andy,

things are getting better. 

There are some small style issues, only some hangups I have that is not important.

However there are still problems with the scripts.

Let's looking at it line by line.


> %pre

%pre is empty, so remove that tag.


> %post
> pdir=%{_sbindir}

This don't make sense, why define pdir as %{_sbindir},
use %{_sbindir} directly (if needed).

> # echo "Installing IPMI Management Utilities %{version} ..."

rpms should not generate any output (unless something wrong happens).
Remove this line. 

> # POST_INSTALL

post install in %post, well we know that, remove.


> # check if chroot, which will not have /proc instances
> if [ -f /proc/devices ]
> then
> 	isroot=1
> else
>	isroot=0
> fi


> if [ "$1" = "1" ]
> then
>  # doing rpm -i, first time
>  if [ $isroot -eq 1 ]
>  then
>     # detects IPMI interface type, output to %{_datadir}/%{name}/ipmi_if.txt
>     %{_datadir}/%{name}/ipmi_if.sh 
>
>     # Run some ipmiutil command to see if any IPMI interface works.
>     $pdir/ipmiutil wdt >/dev/null 2>&1
>     fIPMIret=$?
>  else
>     # else chroot, set as if IPMI is not supported
>     fIPMIret=1
>  fi
>
>  # If fIPMIret==0, the IPMI cmd was successful, and IPMI is enabled.
>  if [ $fIPMIret -eq 0 ] 
>  then

You should move all this logic to the init scripts itself.

When a user is adding the ipmtutil package, he/she will be surprised
if the init scripts are not registered in the chkconfig system.

If something goes when starting e.g. ipmi_port let the scripts
itself return the correct error message and exit status code.

More about init scripts in full glory details here:

 https://fedoraproject.org/wiki/Packaging:SysVInitScript


>     # IPMI_IS_ENABLED
>     # Enable services: ipmi_port reserves the IPMI RMCP port from portmap
>     if [ -x /sbin/chkconfig ]; then
>	/sbin/chkconfig --add ipmi_port
>	/sbin/chkconfig --add ipmiutil_wdt
>	/sbin/chkconfig --add ipmiutil_asy 
>     fi


>     # Capture a snapshot of IPMI sensor data for later reuse.
>     sensorout=%{_datadir}/%{name}/sensor_out.txt
>     if [ ! -f $sensorout ] 
>     then
>        echo "Saving a sensor snapshot to $sensorout ..."
>        $pdir/ipmiutil sensor >$sensorout

Why are you doing this? And again, rpms should not produce any output.


>     fi
>  fi
> fi
> echo "done `date`" 

Please remove this too.


> %preun
> # before uninstall
> echo "Uninstalling ipmiutil-%{version} package ..."

yum/rpm will inform the user any way, remove this


> # arg1 = 1 if rpm -U, arg1 = 0 if rpm -e
> if [ "$1" = "0" ]
>then
>  if [ -x %{_initrddir}/ipmi_port ]
>  then
>     %{_initrddir}/ipmi_port stop	>/dev/null 2>&1
>     %{_initrddir}/ipmiutil_wdt stop	>/dev/null 2>&1
>     %{_initrddir}/ipmiutil_asy stop	>/dev/null 2>&1
>     if [ -x /sbin/chkconfig ]; then
>	/sbin/chkconfig --del ipmi_port     >/dev/null 2>&1
>	/sbin/chkconfig --del ipmiutil_wdt  >/dev/null 2>&1
>	/sbin/chkconfig --del ipmiutil_asy  >/dev/null 2>&1
>     fi
>  fi
>fi

This is better than %post, however not according to policy:

https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_spec_file_scriptlets


> %postun

Same as %pre, remove.


One more thing, I had look at the script

%{_datadir}/%{name}/ipmi_if.txt

First it seems to require dmidecode, so add a requires on the dmidecode package.

Then there is security issue:

dmiout=/tmp/dmi.out

[snip]

dmidecode >$dmiout


Remember the system is running this script as root, if a user does

$ ln -s /etc/passwd /tmp/dmi.out 

before ipmutil is installed, the system is broken after impiutil is installed.

A simple fix is to use this trick:

dmiout=$(mktemp /tmp/impiutil.XXXXXX)


You also have

ifdir=/usr/share/ipmiutil
ifout=$ifdir/ipmi_if.txt

and writes to this file, however you should not write to files in /usr subdirs.
If you want to write to files, use /var/lib/ e.g. 

 /var/lib/ipmiutil/ipmi_if.txt
Comment 32 Andy Cress 2010-05-05 11:18:55 EDT
I have built a test spec file with most of the changes implemented and submitted to SVN.  I'd like this reviewed before producing a release with a src.rpm.  

Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec

I have removed all non-essential %post logic, but the remaining logic cannot be moved to init scripts since init scripts do not auto-start at install time, and the remaining logic should only run once at install time.

The 'chkconfig -add' lines are conditional because, in the use case where the package is being installed as an IPMI LAN client and there is no local IPMI supported, it does not make sense for those services to be loaded or run at all.
Comment 33 Hans de Goede 2010-07-12 06:14:26 EDT
Hi Andy,

If you're still interested in getting ipmiutil into Fedora and maintaining it there, let me know (with a comment here) and I'll take a thorough look at your latest specfile (and if all is well eventually sponsor you).

Regards,

Hans
Comment 34 Andy Cress 2010-07-12 07:58:20 EDT
Hans,

Yes I certainly am interested, thank you. 
Latest spec file and srpm:
Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.6-1.src.rpm

Andy
Comment 35 Hans de Goede 2010-07-12 08:47:28 EDT
Hi Andy,

Here is a full review of this package.

- rpmlint checks return:
 <not run build fails>
- package meets naming guidelines
- package meets packaging guidelines
- license (BSD) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
  FAIL, see below
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
  MUST FIX: %{_datadir}/%{name} is not owned. To fix this either do
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- other remarks:
  I don't like the creation of %{_var}/lib/%{name} from %post, this should
  be done in %install and then added to the file list.
  I would prefer for the extra <space> between the - and the , in %defattr to
  be dropped.

All in all I must say this packages looks good. Except that it does not build,
the initscripts get installed in /etc/init.d but %files looks for them in
/etc/rc.d/init.d (which is the correct location, on Fedora /etc/init.d is a compatibility symlink to /etc/rc.d/init.d .

Summarizing, the following must be fixed:
1. Does not build
2. %{_datadir}/%{name}
3. Creation of %{_var}/lib/%{name} from %post

And the following could be fixed:
1. The extra <space> between the - and the , in %defattr


Normally when we get a new packager for Fedora we require him/her to do a number of package reviews to show understanding of the guidelines. But given that you are upstream for ipmiutils which is sort of special and that the current spec file looks quite good I'm willing to sponsor you once the remaining issues are solved. I've one special request though, if you ever decide to submit another package (please do), then please let me know and I'll look over your shoulder (for the first 1 or 2 other packages).

Regards,

Hans
Comment 36 Andy Cress 2010-07-15 16:51:38 EDT
Hans,

OK, there were some other changes in progress, but that's all ready now.
I made the spec file updates you highlighted.  I guess my build systems always had the /etc/init.d link so I never noticed that failing.  

Latest spec file and srpm:
Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.7-1.src.rpm

Andy
Comment 37 Hans de Goede 2010-07-18 05:52:16 EDT
Hi Andy,

Thanks for the new version. This time it builds. So now I've been able to run rpmlint. rpmlint has found some minor issues once those are fixed I'll approve this package and sponsor you.

First a bunch of rpmlint false-positives for the sake of completeness.
ipmiutil.src: W: spelling-error %description -l en_US metacommand -> meta command, meta-command, Metacom
ipmiutil.src: W: spelling-error %description -l en_US subcommand -> sub command, sub-command, subcommander
ipmiutil.src: W: spelling-error %description -l en_US dev -> dec, deb, de
ipmiutil.src: W: spelling-error %description -l en_US ipmi -> impi
ipmiutil.src: W: spelling-error %description -l en_US imb -> ib, mb, iamb
ipmiutil.x86_64: W: spelling-error %description -l en_US metacommand -> meta command, meta-command, Metacom
ipmiutil.x86_64: W: spelling-error %description -l en_US subcommand -> sub command, sub-command, subcommander
ipmiutil.x86_64: W: spelling-error %description -l en_US dev -> dec, deb, de
ipmiutil.x86_64: W: spelling-error %description -l en_US ipmi -> impi
ipmiutil.x86_64: W: spelling-error %description -l en_US imb -> ib, mb, iamb
ipmiutil.x86_64: W: service-default-enabled /etc/rc.d/init.d/ipmi_port

Then some real (although very minor) issues:
ipmiutil.src:125: W: mixed-use-of-spaces-and-tabs (spaces: line 106, tab: line 125)
ipmiutil.x86_64: W: spurious-executable-perm /usr/share/doc/ipmiutil-2.6.7/ChangeLog
ipmiutil.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/ChangeLog
ipmiutil.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/README
ipmiutil.x86_64: W: spurious-executable-perm /usr/share/doc/ipmiutil-2.6.7/UserGuide
ipmiutil.x86_64: E: wrong-script-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/UserGuide
ipmiutil.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/TODO
ipmiutil.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/COPYING
ipmiutil.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/ipmiutil-2.6.7/AUTHORS

The mixed-use-of-spaces-and-tabs thingie speaks for itself, as for the others,
the usual way to fix this is to do the following in %setup:
chmod -x foo bar
sed -i 's/\r//' foo bar

But given that you are upstream, you could of course also fix these in the tarbal :)

Regards,

Hans
Comment 38 Andy Cress 2010-07-19 12:16:47 EDT
Hans,

I have fixed those issues in these test versions (pre-release):
Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.8-1.src.rpm

BTW, about rpmlint: 
I'm using rpmlint version 0.80.  Which version of rpmlint are you using?  And which rpmlint command-line gets the extra detail you showed?  Perhaps the extra detail only shows up from installing the src rpm, since most of my runs would have been with the binary rpm.
Comment 39 Hans de Goede 2010-07-19 14:41:27 EDT
(In reply to comment #38)
> Hans,
> 
> I have fixed those issues in these test versions (pre-release):
> Spec URL: http://ipmiutil.sourceforge.net/docs/ipmiutil.spec
> SRPM URL: http://ipmiutil.sourceforge.net/FILES/ipmiutil-2.6.8-1.src.rpm
>

Looks good, Approved!

If you create a FAS account (if you haven't already), apply for packager group membership and let me know you FAS username then I'll sponsor you and you can move
on with the CVS request, and then import and build, etc.

> BTW, about rpmlint: 
> I'm using rpmlint version 0.80.  Which version of rpmlint are you using?  And
> which rpmlint command-line gets the extra detail you showed?  Perhaps the extra
> detail only shows up from installing the src rpm, since most of my runs would
> have been with the binary rpm.    

I'm using the rpmlint version which comes with F-13: rpmlint-0.97-1.fc13.noarch

I'm invoking it like this:
rpmlint rpmbuild/RPMS/x86_64/ipmiutil-* rpmbuild/SRPMS/ipmiutil-2.6.8-1.fc13.src.rpm

You can also run rpmlint on an installed package (it will then run another set of tests as when run on source / binary rpms), like this:
rpmlint ipmiutil

Regards,

Hans
Comment 40 Andy Cress 2010-07-19 15:10:35 EDT
Hans,

Thanks. 
My FAS account is arcress, see also https://fedoraproject.org/wiki/User:Arcress.
I had previously applied to be a Packager:
  Fedora Packager CVS Commit Group(user)
  Status:   Unapproved 
From the wiki, it appears that next I need to install Koji.

Andy
Comment 41 Hans de Goede 2010-07-20 04:34:44 EDT
Hi,

(In reply to comment #40)
> Hans,
> 
> Thanks. 
> My FAS account is arcress

Ok, you've been sponsored,

, see also
> https://fedoraproject.org/wiki/User:Arcress.
> I had previously applied to be a Packager:
>   Fedora Packager CVS Commit Group(user)
>   Status:   Unapproved 
> From the wiki, it appears that next I need to install Koji.

The koji client, yes and add a CVS request here for creation of a CVS module
for ipmiutil. When that is done you can then import and build ipmiutil, etc.

Let me know if you have any questions.

Regards,

Hans
Comment 42 Andy Cress 2010-07-20 16:26:51 EDT
New Package CVS Request
=======================
Package Name: ipmiutil
Short Description: A package that provides easy-to-use IPMI server management utilities
Owners: arcress
Branches: F-14 EL-6
InitialCC:
Comment 43 Kevin Fenzi 2010-07-21 01:06:15 EDT
CVS done (by process-cvs-requests.py).

We are not yet doing F-14 branches. Otherwise cvs done.
Comment 44 Andy Cress 2010-07-21 15:09:38 EDT
I did a checkout and did the cvs-import of ipmiutil successfully, but when I tried to do a build, I got an error.  Please help me figure out what is wrong.

[arcress@ac4lht devel]$ make build
rpm: no arguments given for query
 not tagged with tag ipmiutil--
make: *** [build-check] Error 1
[arcress@ac4lht devel]$ pwd
/home/arcress/cvs/ipmiutil/devel
Comment 45 Terje Røsten 2010-07-21 15:12:45 EDT
Thanks for reviewing this package Hans. And Andy for not giving up years ago. 
  
How did you import?
Comment 46 Andy Cress 2010-07-21 16:34:04 EDT
I was following the wiki, and I did:
$ fedora-cvs ipmiutil
(which did the checkout)
$ cd ipmiutil
Then here is the output from the import:

[arcress@ac4lht ipmiutil]$ ./common/cvs-import.sh /tmp/ipmiutil-2.6.8-1.src.rpm
Checking out module: 'ipmiutil'
Unpacking source package: ipmiutil-2.6.8-1.src.rpm...
L ipmiutil-2.6.8.tar.gz
A ipmiutil.spec

Checking : ipmiutil-2.6.8.tar.gz on https://cvs.fedoraproject.org/repo/pkgs/uplo
ad.cgi...
Uploading: ipmiutil-2.6.8.tar.gz to https://cvs.fedoraproject.org/repo/pkgs/uplo
ad.cgi...
######################################################################## 100.0%

Source upload succeeded. Don't forget to commit the new ./sources file
M sources
M .cvsignore
cvs add: scheduling file `./devel/import.log' for addition
cvs add: use 'cvs commit' to add this file permanently
=======================================================================
Index: devel/.cvsignore
===================================================================
RCS file: /cvs/pkgs/rpms/ipmiutil/devel/.cvsignore,v
retrieving revision 1.1
diff -u -r1.1 .cvsignore
--- devel/.cvsignore    21 Jul 2010 05:20:41 -0000      1.1
+++ devel/.cvsignore    21 Jul 2010 17:37:26 -0000
@@ -0,0 +1 @@
+ipmiutil-2.6.8.tar.gz
cvs diff: devel/import.log is a new entry, no comparison available
cvs diff: devel/ipmiutil.spec is a new entry, no comparison available
Index: devel/sources
===================================================================
RCS file: /cvs/pkgs/rpms/ipmiutil/devel/sources,v
retrieving revision 1.1
diff -u -r1.1 sources
--- devel/sources       21 Jul 2010 05:20:41 -0000      1.1
+++ devel/sources       21 Jul 2010 17:37:26 -0000
@@ -0,0 +1 @@
+b124674b28e8da96ead0a8db3a1da3eb  ipmiutil-2.6.8.tar.gz
=======================================================================
Please check the above cvs diff.
If you want to make any changes before committing, please press Ctrl-C.
Otherwise press Enter to proceed to commit.

cvs commit...
**** Access allowed: arcress is in ACL for rpms/ipmiutil/devel.
Checking in devel/.cvsignore;
/cvs/pkgs/rpms/ipmiutil/devel/.cvsignore,v  <--  .cvsignore
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvs/pkgs/rpms/ipmiutil/devel/import.log,v
done
Checking in devel/import.log;
/cvs/pkgs/rpms/ipmiutil/devel/import.log,v  <--  import.log
initial revision: 1.1
done
RCS file: /cvs/pkgs/rpms/ipmiutil/devel/ipmiutil.spec,v
done
Checking in devel/ipmiutil.spec;
/cvs/pkgs/rpms/ipmiutil/devel/ipmiutil.spec,v  <--  ipmiutil.spec
initial revision: 1.1
done
Checking in devel/sources;
/cvs/pkgs/rpms/ipmiutil/devel/sources,v  <--  sources
new revision: 1.2; previous revision: 1.1
done
Running syncmail...
Mailing scm-commits@lists.fedoraproject.org ipmiutil-owner@fedoraproject.org...
...syncmail done.
Running syncmail...
Mailing relnotes@fedoraproject.org...
...syncmail done.
Commit Complete
cvs tag  -c ipmiutil-2_6_8-1_fc14
cvs tag: Tagging .
T .cvsignore
T Makefile
T import.log
T ipmiutil.spec
T sources
Tagged with: ipmiutil-2_6_8-1_fc14

Tagging complete.

[arcress@ac4lht ipmiutil]$
Comment 47 Terje Røsten 2010-07-21 23:01:09 EDT
Try:

 cvs update -dP -A

in ipmiutil dir, then cd devel; make build.
Comment 48 Hans de Goede 2010-07-22 05:10:20 EDT
(In reply to comment #47)
> Try:
> 
>  cvs update -dP -A
> 
> in ipmiutil dir, then cd devel; make build.    

Right, I think you are missing ipmiutil.spec in the devel dir, because you did not do a cvs upd after the import. Note that the import script creates a tmp dir and works from there.

Regards,

Hans
Comment 49 Andy Cress 2010-07-22 12:11:00 EDT
Much better, thanks.  A successful rawhide build.

[arcress@ac4lht ipmiutil]$ cvs update -dP -A
cvs update: Updating .
cvs update: Updating EL-6
cvs update: Updating common
cvs update: Updating devel
U devel/.cvsignore
U devel/import.log
U devel/ipmiutil.spec
U devel/sources
[arcress@ac4lht ipmiutil]$ cd devel
[arcress@ac4lht devel]$ make build
/usr/bin/koji  build  dist-rawhide 'cvs://cvs.fedoraproject.org/cvs/pkgs?rpms/ip
miutil/devel#ipmiutil-2_6_8-1_fc14'
Created task: 2341823
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=2341823
None
Watching tasks (this may be safely interrupted)...
2341823 build (dist-rawhide, /cvs/pkgs:rpms/ipmiutil/devel:ipmiutil-2_6_8-1_fc14
): open (x86-09.phx2.fedoraproject.org)
  2341825 buildSRPMFromSCM (/cvs/pkgs:rpms/ipmiutil/devel:ipmiutil-2_6_8-1_fc14)
: open (x86-12.phx2.fedoraproject.org)
  2341825 buildSRPMFromSCM (/cvs/pkgs:rpms/ipmiutil/devel:ipmiutil-2_6_8-1_fc14)
: open (x86-12.phx2.fedoraproject.org) -> closed
  0 free  1 open  1 done  0 failed
  2341879 buildArch (ipmiutil-2.6.8-1.fc14.src.rpm, i686): open (x86-12.phx2.fed
oraproject.org)
  2341878 buildArch (ipmiutil-2.6.8-1.fc14.src.rpm, x86_64): open (x86-20.phx2.f
edoraproject.org)
  2341878 buildArch (ipmiutil-2.6.8-1.fc14.src.rpm, x86_64): open (x86-20.phx2.f
edoraproject.org) -> closed
  0 free  2 open  2 done  0 failed
  2341913 tagBuild (noarch): closed
  2341879 buildArch (ipmiutil-2.6.8-1.fc14.src.rpm, i686): open (x86-12.phx2.fed
oraproject.org) -> closed
  0 free  1 open  4 done  0 failed
2341823 build (dist-rawhide, /cvs/pkgs:rpms/ipmiutil/devel:ipmiutil-2_6_8-1_fc14
): open (x86-09.phx2.fedoraproject.org) -> closed
  0 free  0 open  5 done  0 failed

2341823 build (dist-rawhide, /cvs/pkgs:rpms/ipmiutil/devel:ipmiutil-2_6_8-1_fc14
) completed successfully
[arcress@ac4lht devel]$
Comment 50 Andy Cress 2010-07-23 13:55:50 EDT
OK, now that I have a successful rawhide build, and it passes the testing I have run, is there anything else I need to do before the Fedora 14 feature freeze (July 27) to make sure that it is included?
Comment 51 Hans de Goede 2010-07-23 15:04:03 EDT
Hi,

(In reply to comment #50)
> OK, now that I have a successful rawhide build, and it passes the testing I
> have run, is there anything else I need to do before the Fedora 14 feature
> freeze (July 27) to make sure that it is included?    

No,

You can even introduce it in F-13 as an update if you want, request an F-13 branch, then copy all files from the devel dir in cvs over to the F-13 branch, cvs add the new ones and commit, tag and build. Once build you can then rfequest the new build to be added to the updates repository through bodhi.

Regards,

Hans
Comment 52 Andy Cress 2010-10-11 12:06:05 EDT
Hans,

I have a new version 2.7.x with enhancements that I would like to update into the CVS rawhide branch targeted for EL6.  The wiki isn't very clear on this procedure, and it seems to be in transition between cvs and git syntax also.  
Since the cvs directory does not contain individual source files, should I re-import a new src.rpm?  What is the correct procedure?

Andy
Comment 53 Hans de Goede 2010-10-11 13:45:36 EDT
(In reply to comment #52)
> Hans,
> 
> I have a new version 2.7.x with enhancements that I would like to update into
> the CVS rawhide branch targeted for EL6.  The wiki isn't very clear on this
> procedure, and it seems to be in transition between cvs and git syntax also.  
> Since the cvs directory does not contain individual source files, should I
> re-import a new src.rpm?  What is the correct procedure?
> 
> Andy

Hi,

I'm not all that familiar with EPEL I'm afraid is EPEL still using CVS?

If so then using cvs-import.sh on a new srpm is probably the easiest way to update.

Regards,

Hans

p.s.

You can always ask questions like these in the #fedora-devel channel on the freenode irc network.
Comment 54 Dan Horák 2010-10-11 15:24:12 EDT
(In reply to comment #52)
> Hans,
> 
> I have a new version 2.7.x with enhancements that I would like to update into
> the CVS rawhide branch targeted for EL6.  The wiki isn't very clear on this
> procedure, and it seems to be in transition between cvs and git syntax also.  
> Since the cvs directory does not contain individual source files, should I
> re-import a new src.rpm?  What is the correct procedure?

EPEL lives in the same git repo as Fedora branches, do "fedpkg switch-branch el6" and you have content the EL-6 branch. Because it's empty you can use "fedpkg import --branch el6 your.srpm" or just copy the ipmiutil.spec and sources from the master branch and continue with commit and push.

And as Hans pointed out - #fedora-devel (or #epel) is the best place to get the answers quickly.
Comment 55 Hans de Goede 2010-11-05 08:49:01 EDT
Closing this bug, as ipmiutil has been built for f14, f15 and epel6.
Comment 56 Andy Cress 2014-11-25 21:54:34 EST
Package Change Request
======================
Package Name:  ipmiutil
New Branches:  epel7
Owners:  arcress (Andy Cress)
Comment 57 Gwyn Ciesla 2014-11-26 07:29:27 EST
Git done (by process-git-requests).

Please make the owners entry fas username only.

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