Bug 368401 - Review Request: freeipmi - Free IPMI userland implementation
Summary: Review Request: freeipmi - Free IPMI userland implementation
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Karsten Hopp
QA Contact: Fedora Extras Quality Assurance
Depends On: 368541
TreeView+ depends on / blocked
Reported: 2007-11-06 15:20 UTC by Phil Knirsch
Modified: 2015-03-05 01:19 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-08-18 12:33:21 UTC
Type: ---
karsten: fedora-review+
dennis: fedora-cvs+

Attachments (Terms of Use)

Description Phil Knirsch 2007-11-06 15:20:24 UTC
Spec URL: http://pknirsch.fedorapeople.org/freeipmi.spec
SRPM URL: http://pknirsch.fedorapeople.org/freeipmi-0.4.6-1.fc7.src.rpm
The FreeIPMI project provides "Remote-Console" (out-of-band) and
"System Management Software" (in-band) based on Intelligent
Platform Management Interface specification.

Comment 1 Karsten Hopp 2007-11-06 15:25:20 UTC
This doesn't build on Rawhide

gcc -DHAVE_CONFIG_H -I. -I. -I../..  -I./../../common/src -D_GNU_SOURCE -I./../../libfreeipmi/include   -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -MT 
bmc-config-utils.o -MD -MP -MF ".deps/bmc-config-utils.Tpo" -c -o 
bmc-config-utils.o bmc-config-utils.c; \
        then mv -f ".deps/bmc-config-utils.Tpo" ".deps/bmc-config-utils.Po"; 
else rm -f ".deps/bmc-config-utils.Tpo"; exit 1; fi
In function 'open',
    inlined from 'bmc_config_args_validate' at bmc-config-argp.c:326:
/usr/include/bits/fcntl2.h:51: error: call to '__open_missing_mode' declared 
with attribute error: open with O_CREAT in second argument needs 3 arguments
make[3]: *** [bmc-config-argp.o] Error 1

Comment 2 Bill Nottingham 2007-11-06 15:26:33 UTC
Relationship to OpenIPMI?

Comment 3 Phil Knirsch 2007-11-06 15:33:07 UTC
FreeIPMI is the userland only implementation of IPMI, no relation to OpenIPMI
except that both are IPMI implementations.

Read ya, Phil

Comment 4 Phil Knirsch 2007-11-06 15:33:51 UTC
And FreeIPMI contains several features of IPMI that OpenIPMI doesn't provide.

Read ya, Phil

Comment 5 Karsten Hopp 2007-11-06 16:44:34 UTC
# /etc/init.d/freeipmi-bmc-watchdog start
prints out the bmc-watchdog help instead of starting it.

a koji build attempt failed: 
gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mminimal-toc -o .libs/bmc-watchdog 
bmc_watchdog-bmc-watchdog.o  ../../libfreeipmi/src/.libs/libfreeipmi.so -lgcrypt -lm 
../../libfreeipmi/src/.libs/libfreeipmi.so: undefined reference to `inb'
../../libfreeipmi/src/.libs/libfreeipmi.so: undefined reference to `outb'
../../libfreeipmi/src/.libs/libfreeipmi.so: undefined reference to `iopl'

The spec file and the package looks good now as all verbal feedback has been 
added. So this package will be approved when the two remaining issues have 
been fixed.

Comment 6 Phil Knirsch 2007-11-06 17:02:45 UTC
First issue fixed.

Second one seems to that sys/io.h is missing on ppc64 from glibc-headers,
opening a bug against glibc and using an ExludeArch: ppc64 ppc  until this is
cleared up.

Read ya, Phil

PS: Fixed several other minor issues as well in the last version (removed static
libs and added several needed Requires(post|preun) among others).

Comment 7 Phil Knirsch 2007-11-06 17:08:13 UTC
Added new glibc bug as depends to this bugzilla.

Read ya, Phil

Comment 8 Karsten Hopp 2007-11-07 09:54:51 UTC
review complete

Comment 9 Dennis Gilmore 2007-11-07 10:04:52 UTC
because the header is missing from ppc64  is no reason to exclude ppc also.  

can you provide links to newer srpms ?

Comment 10 Phil Knirsch 2007-11-07 10:55:36 UTC
According to Jakubs comment here
https://bugzilla.redhat.com/show_bug.cgi?id=368541#c2 i've now added the
following line to the specfile with comment and reference:

ExclusiveArch: %{ix86} x86_64 ia64 alpha

Newest specfile and SRPM have just been uploaded.

Read ya, Phil

Comment 11 Dennis Gilmore 2007-11-07 11:32:32 UTC
again post links to updated spec and srpm  you must bump release for each 
revision. even during review.

Comment 12 Phil Knirsch 2007-11-07 12:25:19 UTC
Can you please reference this in the ReviewGuidelines? I read through them and
didn't find any mention of this there.

It's not a problem to do so, but i wasn't aware that this is a MUST requirement.


Read ya, Phil

Comment 13 Phil Knirsch 2007-11-07 12:48:44 UTC
New Package CVS Request
Package Name: freeipmi
Short Description: Free IPMI userland implementation
Owners: pknirsch
Branches: F-8
Cvsextras Commits: yes

Comment 14 Dennis Gilmore 2007-11-07 13:03:35 UTC
Im not satisfied this has received a proper review

Comment 15 Phil Knirsch 2007-11-07 13:09:04 UTC
Can i get an answer to my question from comment #12 please?


Read ya, Phil

Comment 16 Phil Knirsch 2007-11-07 13:13:11 UTC
rpmlint output of current packages:

[pknirsch@hamburg rpm]$ ls /home/devel/pknirsch/rpm/rpms/i386/
[pknirsch@hamburg rpm]$ rpmlint -i /home/devel/pknirsch/rpm/rpms/i386/*
[pknirsch@hamburg rpm]$ 

So no warning nor error from rpmlint.

Read ya, Phil

Comment 17 Karsten Hopp 2007-11-07 14:07:51 UTC
Ok, here's the list of what I've checked, although this list isn't required by 
the ReviewGuidelines, it just has to be followed (which I did):
- rpmlint: freeipmi.i386: W: incoherent-version-in-changelog 0.4.6-1.fc7 
  I think this one can be neglected, rpmlint should ignore dist tags
- package follows package nameing guidelines
- spec file matches base package name
- Licenses of all files have been checked
- no pre-built binaries included
- package follows FHS
- changelog looks sane
- spec file tags are ok
- buildroot is ok
- buildroot cleaned before install, spec hat a %clean section
- Buildrequirements are ok, no regressions between builds in mock and in a 
full environment, configure output has been checked
- Requires post/preun have been added for programs used in scriptlets
- static libraries have been disabled
- check-rpaths doesn't complain
- config files are noreplace
- initscripts aren't config files
- file permissions look sane
- %makeinstall isn't used
- tarball sha1sum matches upstream's tarball
- smp flags are used for build
- package owns oll directories it creates or requires other packages who own 
- package build in mock and in koji, spec file has an ExclusiveArch because of
  glibc not supporting some required functions on s390x, sparc, ppc* with a 
comment pointing at a bugzilla for the reasons
- freeipmi packages don't have any localizes stuff
- packages run ldconfig in their scriptlets
- no duplicate files in the files sections
- header files are in a -devel subpackage
- packages don't have any .la or pkgconfig files

Btw: it would have been faster if you actually would have taken a quick look 
at the spec file instead of simply rejecting it. Be assured that I didn't 
simply waive it just because Phil happens to sit at the other side of my desk. 
He really had to fix quite a few issues.

My fedora-review+ still stands and if you have any issues with that, take it 
to FESCO instead of mistrusting my review of this package

Comment 18 Dennis Gilmore 2007-11-07 14:39:04 UTC
my main concern was that we have no idea what your issues were and how they 
were fixed because they were not documented properly 

disttag values should not be put into changelogs 


Comment 19 Phil Knirsch 2007-11-07 14:43:59 UTC
Ok, removing distag from changelog then for the final package.

Thanks Karsten and Dennis.

Read ya, Phil

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