[Bug 469931] Review Request: ipmiutil - IPMI Management Utilities

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=469931

Terje Røsten <terjeros at phys.ntnu.no> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?                   |

--- Comment #28 from Terje Røsten <terjeros at phys.ntnu.no> 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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.