Bug 466762

Starting full review. . .

rpmlint on srpm clean

On rpms:

ipmitool.i386: W: service-default-enabled /etc/rc.d/init.d/ipmievd
The service is enabled by default after "chkconfig --add"; for security
reasons, most services should not be. Use "-" as the default runlevel in the
init script's "chkconfig:" line and/or remove the "Default-Start:" LSB keyword
to fix this if appropriate for this service.

Is this really necessary?

ipmitool.i386: W: incoherent-init-script-name ipmievd
The init script name should be the same as the package name in lower case, or
one with 'd' appended if it invokes a process by that name.

Fixable, or does this coorespond with what's in openipmi-tools?

I see no upstream future in the patch, but you might want to comment on that in the spec, just the same.

Running a mock build to test BuildRequires, but otherwise, other than the above, it looks great on full review.

BRs are fine.

> service-default-enabled /etc/rc.d/init.d/ipmievd

I agree that it should not be.  Disabling its autostart is part of the patch submitted in bug 466343:

-# chkconfig: 345 99 00
+# chkconfig: - 99 00

> incoherent-init-script-name ipmievd
> Fixable, or does this coorespond with what's in openipmi-tools?

IMHO, there's nothing to fix here - the init script specifically starts ipmievd, not ipmitool... rpmlint is being a bit silly here.


"Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides: OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages)

From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi support explicitly disabled for reproducible builds.

From bug 466343: configure with --disable-dependency-tracking for slight build speedup and cleaner build logs?

Source0 URL does not match recommended sf.net one: http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

Wouldn't configuring with --enable-file-security be desirable?

"Requires: OpenIPMI" missing?

(In reply to comment #4)
> > service-default-enabled /etc/rc.d/init.d/ipmievd
> 
> I agree that it should not be.  Disabling its autostart is part of the patch
> submitted in bug 466343:
> 
> -# chkconfig: 345 99 00
> +# chkconfig: - 99 00
>
> > incoherent-init-script-name ipmievd
> > Fixable, or does this coorespond with what's in openipmi-tools?
> 
> IMHO, there's nothing to fix here - the init script specifically starts
> ipmievd, not ipmitool... rpmlint is being a bit silly here.

Agreed.
 
> 
> "Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides:
> OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see
> http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages)
> 
> From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi
> support explicitly disabled for reproducible builds.
> 
> From bug 466343: configure with --disable-dependency-tracking for slight build
> speedup and cleaner build logs?

Works for me.

> Source0 URL does not match recommended sf.net one:
> http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

The DNS resolves the same way, but there's no harm fixing it.

> Wouldn't configuring with --enable-file-security be desirable?

Possbily, not sure of the ramifications.  Jan?
 
> "Requires: OpenIPMI" missing?

Probably, yes, I think I misunderstood the relationship here.  Not needed to build, but it would require at runtime, no?

(In reply to comment #5)
> (In reply to comment #4)
> > > service-default-enabled /etc/rc.d/init.d/ipmievd
> > 
> > I agree that it should not be.  Disabling its autostart is part of the patch
> > submitted in bug 466343:
> > 
> > -# chkconfig: 345 99 00
> > +# chkconfig: - 99 00
> >
> > > incoherent-init-script-name ipmievd
> > > Fixable, or does this coorespond with what's in openipmi-tools?
> > 
> > IMHO, there's nothing to fix here - the init script specifically starts
> > ipmievd, not ipmitool... rpmlint is being a bit silly here.
> 
> Agreed.

Rpmlit checks also Default-Start, I'll remove the runlevels from there.

> > "Obsoletes: OpenIPMI-tools < 2.0.14-3" missing (and possibly "Provides:
> > OpenIPMI-tools = 2.0.14-3" if you think it's necessary, see
> > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Renaming.2Freplacing_existing_packages)

Obsoletes: added, Provides: sounds silly to me.

> > From bug 466343: "BuildRequires: freeipmi-devel" should be added, or freeipmi
> > support explicitly disabled for reproducible builds.

Here I am not sure if we want dependency on FreeIPMI... It was not there before and most users won't need it. I'll check it together with pknirsch and we'll decide.

> > From bug 466343: configure with --disable-dependency-tracking for slight build
> > speedup and cleaner build logs?
> 
> Works for me.

Done.

> > Source0 URL does not match recommended sf.net one:
> > http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net
> 
> The DNS resolves the same way, but there's no harm fixing it.

Fixed.
 
> > Wouldn't configuring with --enable-file-security be desirable?
> 
> Possbily, not sure of the ramifications.  Jan?

It just adds additional checks when opening files (like owner, inode, link count etc.). It does not harm nor introduce any performance impact. I'll add it there. 

> > "Requires: OpenIPMI" missing?
> 
> Probably, yes, I think I misunderstood the relationship here.  Not needed to
> build, but it would require at runtime, no?

AFAIK, ipmitools talks to OpenIPMI driver via /dev/ipmi0. It does not link with OpenIPMI libraries. One may use ipmitool when talking to remote host over IP, completely without need of OpenIMPI.


I'll check the freeipmi-devel issue and post new .spec.

Enabling FreeIPMI support would IPMItool possibility to talk to local devices, that are supported by the FreeIPMI driver. But FreeIPMI package itself already provides IPMItool-like utility and there is no need to use IPMItool, that's why I decided to disable it.

In addition to above, I added 'Provides: OpenIPMI-tools = 2.0.14-3' to make rpmlint happy.

You can find new .spec and .srpm at http://people.redhat.com/jsafrane/ipmitool/2/

[limb@fawkes fedora]$ diff ipmitool.spec ~/rpmbuild/SPECS/ipmitool.spec
11c11
< BuildRequires: openssl-devel readline-devel ncurses-devel
---
> BuildRequires: openssl-devel readline-devel ncurses-devel freeipmi-devel

Which is the one I should be using?

oops, I don't know what went wrong, but the srpm has old (=wrong) .spec file. I uploaded new .srpm.

Awesome.

APPROVED.

New Package CVS Request
=======================
Package Name: ipmitool
Short Description: Utility for IPMI control
Owners: jsafrane
Branches: F-10
InitialCC: 

I hope I can still catch F-10.

cvs done.

Thanks for review!