Bug 368401 - Review Request: freeipmi - Free IPMI userland implementation
Review Request: freeipmi - Free IPMI userland implementation
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Karsten Hopp
Fedora Extras Quality Assurance
:
Depends On: 368541
Blocks:
  Show dependency treegraph
 
Reported: 2007-11-06 10:20 EST by Phil Knirsch
Modified: 2015-03-04 20:19 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-18 08:33:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
karsten: fedora‑review+
dennis: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Phil Knirsch 2007-11-06 10:20:24 EST
Spec URL: http://pknirsch.fedorapeople.org/freeipmi.spec
SRPM URL: http://pknirsch.fedorapeople.org/freeipmi-0.4.6-1.fc7.src.rpm
Description: 
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 10:25:20 EST
This doesn't build on Rawhide

if 
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 10:26:33 EST
Relationship to OpenIPMI?
Comment 3 Phil Knirsch 2007-11-06 10:33:07 EST
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 10:33:51 EST
And FreeIPMI contains several features of IPMI that OpenIPMI doesn't provide.

Read ya, Phil
Comment 5 Karsten Hopp 2007-11-06 11:44:34 EST
MUSTFIX:
# /etc/init.d/freeipmi-bmc-watchdog start
prints out the bmc-watchdog help instead of starting it.

a koji build attempt failed: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=228149 
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 12:02:45 EST
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 12:08:13 EST
Added new glibc bug as depends to this bugzilla.

Read ya, Phil
Comment 8 Karsten Hopp 2007-11-07 04:54:51 EST
review complete
Comment 9 Dennis Gilmore 2007-11-07 05:04:52 EST
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 05:55:36 EST
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 06:32:32 EST
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 07:25:19 EST
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.

Thanks,

Read ya, Phil
Comment 13 Phil Knirsch 2007-11-07 07:48:44 EST
New Package CVS Request
=======================
Package Name: freeipmi
Short Description: Free IPMI userland implementation
Owners: pknirsch
Branches: F-8
InitialCC:
Cvsextras Commits: yes
Comment 14 Dennis Gilmore 2007-11-07 08:03:35 EST
Im not satisfied this has received a proper review
Comment 15 Phil Knirsch 2007-11-07 08:09:04 EST
Can i get an answer to my question from comment #12 please?

Thanks,

Read ya, Phil
Comment 16 Phil Knirsch 2007-11-07 08:13:11 EST
rpmlint output of current packages:

[pknirsch@hamburg rpm]$ ls /home/devel/pknirsch/rpm/rpms/i386/
freeipmi-0.4.6-1.fc7.i386.rpm
freeipmi-bmc-watchdog-0.4.6-1.fc7.i386.rpm
freeipmi-debuginfo-0.4.6-1.fc7.i386.rpm
freeipmi-devel-0.4.6-1.fc7.i386.rpm
freeipmi-ipmidetectd-0.4.6-1.fc7.i386.rpm
freeipmi-ipmimonitoring-0.4.6-1.fc7.i386.rpm
[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 09:07:51 EST
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 
0.4.6-1
  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 
them
- 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 09:39:04 EST
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 

CVS-Done
Comment 19 Phil Knirsch 2007-11-07 09:43:59 EST
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.