Not a review, but a couple thoughts -- which I'll preface by saying I'd love to
see this one in extras <grin>:

* As this isn't a kernel or a kernel module, a group of "Applications/System" is
probably more appropriate than "System Environment/Kernel"

* I'm pretty sure the license isn't GPL, as, e.g., ibmasm/src/rsa.h states:
"This software may be used and distributed according to the terms of the Lesser
GNU Public License, incorporated herein by reference".

* A full upstream source URL is required, such that the code tarball can be
fetched independently.

* With respect to "ExclusiveArch: i386", why exclude x86_64?  Is this documented
somewhere?

---

On a different level, why not make this a public bug?

(In reply to comment #1)
> Not a review, but a couple thoughts -- which I'll preface by saying I'd love to
> see this one in extras <grin>:
> 
> * As this isn't a kernel or a kernel module, a group of "Applications/System" is
> probably more appropriate than "System Environment/Kernel"

OK. Will change.
> 
> * I'm pretty sure the license isn't GPL, as, e.g., ibmasm/src/rsa.h states:
> "This software may be used and distributed according to the terms of the Lesser
> GNU Public License, incorporated herein by reference".

Uhuh. Let me speak with the author to get clarificaton on this.
> 
> * A full upstream source URL is required, such that the code tarball can be
> fetched independently.

Working on having it in sourceforge

> 
> * With respect to "ExclusiveArch: i386", why exclude x86_64?  Is this documented
> somewhere?

No/Yes. The ibmasm module is only enabled for i386. The RSA(1) adapter was never
shipped in machines that support 64-bit mode, so no need for 64-bit package.
> 
> ---
> 
> On a different level, why not make this a public bug?

Hmm. Good thought - seems that by mistake I had it set to Fedora contributors only.

Are you sponsored for fedora extras, with cvsextras group membership?  If not,
you should set this bug to block FE-NEEDSPONSOR as well, so the right people who
can do that are aware....

Update:

Registration is pending on sourceforge. Once that is up, I will re-do the
package to have the right Source.

In the meantime, I updated the spec with comments, patched it a bit to compile
it cleanly under FC5 (had some warnings)

Spec URL: http://www.darnok.org/ibmasm/3.0-5/SRPMS/ibmasm.spec
SRPM URL: http://www.darnok.org/ibmasm/3.0-5/i386/ibmasm-3.0-5.i386.rpm

Created attachment 131048 [details]
Dmesg output after installing ibmasm under FC5 - SELinux label error

SELinux does not have ibmasmfs labelled.

FYI: I tested the ibmasm-3.0-5 on a fresh FC5 install with selinux=0 and the
package works.

The packages are up on the sourceforge website.

Spec URL: http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPM URL: http://prdownloads.sourceforge.net/ibmasm/ ibmasm-3.0-6.src.rpm?download

What else is required to have this package be part in FC?

- The Release-tag misses %{?dist}, e.g. Release: 6%{?dist}
- The spec file does not use %{optflags}, so nothing is compiled with the 
typical flags of Fedora that include some security relevant flags.
You can change this, e.g. with export CFLAGS="%{optflags}" prior to the first 
make and patch the Makefiles to make the compiler use CFLAGS
- You need to run ldconfig since you install an .so file in /lib.
- The License should be installed in the docdir and included in the upstream 
source package if it is not.

Btw. you just need to wait until an official reviewer reviews your package and 
a Sponsor sponsers you. They will also tell you what else you need to change, 
if there is anything.

Updated the spec file (per suggestions in comment #8) and the source code to
compile more cleanly now.

SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPM URL: http://prdownloads.sourceforge.net/ibmasm/ibmasm-3.0-6.src.rpm?download

Also adding Aristeu who is the maintainner of ibmasm for RHEL4 and RHEL5.

err, correction (wrong version):

SRPMS URL: http://prdownloads.sourceforge.net/ibmasm/ibmasm-3.0-7.src.rpm?download

Konrad, have you read through
http://fedoraproject.org/wiki/Extras/HowToGetSponsored ?  It's obvious that
you're actively maintaining this package and responding to comments, but
prospective sponsors will probably want to see some reviews or other packages. 
If you have something else you'd like to submit, go ahead and send it in. 
Otherwise, consider commenting on some other open reviews as Till did above.

Please, when you provide a SRPM URL, you give the full URL. I do a heck of a 
lot of tests via ssh and download srpms using wget. I'm not bothered which 
mirror you use, but in future, give the full URL.

rpmlint on the binary has shown problems which need addressing before you 
progress

W no-soname /lib/libsysSP.so
E shlib-with-non-pic-code /lib/libsysSp.so
E shared-lib-without-dependency-information /lib/libsysSp.so
E non-standard-executable-perm /usr/sbin/ibmspup 0744
E non-readable /usr/sbin/ibmsphalt 0700
E non-standard-executable-perm /usr/sbin/ibmsphalt 0700
W no-reload-entry /etc/rc.d/init.d/ibmasm
E incoherent-subsys /etc/rc.d/init.d/ibmasm ibmsphalt

Other notes

You need to put a blocker on this for x86_64 and explain why
You should be consistent on how you use $RPM_BUILD_ROOT - have everything as 
${RPM_BUILD_ROOT} or $RPM_BUILD_ROOT (or better still, %{buildroot}) but not a 
mixture
mkdir -p $RPM_BUILD_ROOT/usr/bin should be $RPM_BUILD_ROOT%{_sbindir}
You don't seem to have added anywhere the runlevels this should run at
The %doc can be on one line
You need either to change the /sbin/ldconfig to read
%post -p /sbin/ldconfig
or add BR ldconfig
Similarly, you need chkconfig adding as a BR

As I said in #12, please remember to post the full URL of the updated SRPM and 
spec file.

(In reply to comment #13)
> You need to put a blocker on this for x86_64 and explain why

+1 on the explanation; as far as I can see this restriction makes sense as i386
kernels do have ibmasm.ko whereas, e.g., x86_64 kernels don't.  (and this
package depends on that kmod.)

Paul,

I better upgrade rpmlint. Mine shows no problems. I will fix the problems you
have and get back to you in a week timeframe with the updated URL that will
contain fixes.

Thanks. While fixing rpmlint, any chance of making it complain if a file doesn't
exist (such as typing rpmlint rpmbuild/i386/foo.i386.rpm when it should be
rpmbuild/rpms/i386/foo.i386.rpm)?

(In reply to comment #16)
> While fixing rpmlint, any chance of making it complain if a file doesn't
> exist (such as typing rpmlint rpmbuild/i386/foo.i386.rpm when it should be
> rpmbuild/rpms/i386/foo.i386.rpm)?

Not sure why this was requested in this particular bug report, but since I
happened to notice it: see bug 198707

#15 : any sign of the update?

(In reply to comment #18)
> #15 : any sign of the update?

<sigh> Not yet. I will work on it today and have it ready by Monday. Sorry for
the delay.

SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPM URL: http://osdn.dl.sourceforge.net/sourceforge/ibmasm/ibmasm-3.0-8.src.rpm

> You don't seem to have added anywhere the runlevels this should run at
This one I was not sure about. Initially it was set to run at level 3. But it
fails if your machine does not have an IBM RSA card installed. Since many
machines don't have this, the runlevels in RHEL4/RHEL5 were removed so that
the system admin would have to do this himself/herself. 

I can make it to be in runlevel 3 again, since this package would be initially
in  Fedora Extras and then hopefully moved to Core (at which point it definitly
should not set the runlevel and let the system admin do it - or perhaps do a
lspci and check to see if the PCI device is installed?).

I would run lspci first to check for the device and then if it's there set the
run level.

I'm not exactly happy at making it run level 3 for extras and then changing it
for Core (or RHEL) - it should be correct in all versions before being accepted.

Paul,

Sounds good. I made a fix so that it would check the PCI Vendor ID and if found,
add the service to the proper run-level.

Here are the updated URLs:
SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPMS URL: http://osdn.dl.sourceforge.net/sourceforge/ibmasm/ibmasm-3.0-9.src.rpm

Builds cleanly, rpmlint complains on the -devel package

W: summary-ended-with-dot
E: summary-too-long
W: no-docs (not worried by)
E: script-without-shellbank %{_includedir}/ibmasm/rsa.h and libibmasm.h

The errors need fixing (the last one is probably just a case of setting the
permission to 644 for each file)

Not building in mock

Executing /usr/sbin/mock-helper chroot
/var/lib/mock/fedora-development-i386-core/root /sbin/runuser - root -c
"/sbin/runuser -c 'rpmbuild -ba --target i386 --nodeps
/builddir/build/SPECS/ibmasm.spec' mockbuild"
error: File /builddir/build/SOURCES/ibmasm_user_3.0-9.fc6.tar.bz2: No such file
or directory

(this could well be my system, but it's working on other packages correctly)

(In reply to comment #23)
> Builds cleanly, rpmlint complains on the -devel package

Oh, I completly forgot to run rpmlint on the -devel package. Thank you for
spotting that.

> 
> W: summary-ended-with-dot
> E: summary-too-long
> W: no-docs (not worried by)
> E: script-without-shellbank %{_includedir}/ibmasm/rsa.h and libibmasm.h
> 
> The errors need fixing (the last one is probably just a case of setting the
> permission to 644 for each file)

Done.
> 
> Not building in mock
> 
> Executing /usr/sbin/mock-helper chroot
> /var/lib/mock/fedora-development-i386-core/root /sbin/runuser - root -c
> "/sbin/runuser -c 'rpmbuild -ba --target i386 --nodeps
> /builddir/build/SPECS/ibmasm.spec' mockbuild"
> error: File /builddir/build/SOURCES/ibmasm_user_3.0-9.fc6.tar.bz2: No such file
> or directory
> 
> (this could well be my system, but it's working on other packages correctly)

This is due to:
Release: 9%{?dist}
and 
Source0: ibmasm_user_%{version}-%{release}.tar.bz

Should I just remove the %{?dist} out? I decided to do that for the .10 release.
It can always be changed back, or there can be two types of Release: tags?


Here is the 3.0-10 release with fixes.

SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPMS URL: http://osdn.dl.sourceforge.net/sourceforge/ibmasm/ibmasm-3.0-10.src.rpm

There should only be one release tag and (from what I've been told from other RH
engineers) the dist tag is optional. I would want a dist tag. The tarball should
be %{name}-%{version}.tar.bz2 (or similar). The %{release} should really be a
number rather than a macro. Yes, it's a pain, but it solves both problems.

%package devel
Requires: %{name} = %{version}

You need this to be %{version}-%{release} to keep things in line

%install
make _LIB=%{_libdir} ROOT=${RPM_BUILD_ROOT} install

Will the package not let you use make DEST_DIR=%{buildroot} install?

install ../ibmasm.init ${RPM_BUILD_ROOT}%{_sysconfdir}/rc.d/init.d/ibmasm

Please use -m for the permission (I have a feeling it's a blocker if you don't
use it). You have in the %files devel

%attr(644,root,root) %{_includedir}/ibmasm/*.h

If you use install -m 0644 in the %install step, the %attr is not required.

%{_sbindir}/ibmsphalt
%{_sbindir}/ibmspup

Can be globbed - %{_sbindir}/ibms*

The package is happy in mock (from your srpm)

Correct these and I'll do the full review - I can't see many more problems though.

Paul,
A week passed so fast for me. I will get this done by Wednesday.

>make _LIB=%{_libdir} ROOT=${RPM_BUILD_ROOT} install
>Will the package not let you use make DEST_DIR=%{buildroot} install?

That is correct. It has the ROOT defined.

> [... other reviews ... ]

I fixed the dependency, permissions, globbed the ibms*

SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPMS URL: http://osdn.dl.sourceforge.net/sourceforge/ibmasm/ibmasm-3.0-11.src.rpm

Builds fine in mock, rpmlint is quiet.

Full review time

Spec file clear, in US English
License is good and the exclusivearch is commented - you will need to add a
blocked on 64 bit architectures though via bugzilla
Consistent use of macros
Init scripts correctly used
pre/post scripts look good
Builds cleanly in mock
rpmlint quiet
includes devel file, no pc file so no pkg-config required
devel package contains the correct information
documentation include (though not for the devel package - this is okay)

Needs work
-devel 
needs the line %{_includedir}/ibmasm/*.h changing to %{_includedir}/ibmasm
otherwise you the package won't own the directory
-permissions
should be the same for both packages

Do these changes and I'm happy. You will still need a sponsor before you can
import this package.

Paul,

Thank you for your review. I will start looking at the need work items and try
to get them done as soon as holiday schedule permits.

Ping - has anything happened with this?

Hey Paul,

Thanks for pinging me. Here are the updates.

SPEC URL:http://ibmasm.cvs.sourceforge.net/*checkout*/ibmasm/ibmasm/ibmasm.spec
SRPMS URL: http://osdn.dl.sourceforge.net/sourceforge/ibmasm/ibmasm-3.0-12.src.rpm 


I was not entirely sure what you meant by permissions. The header files are 644.
Should they be 755?

headers should be 644. Something must have gone odd on my box. No worries ;-)

Current version builds fine in and out of mock, doesn't have any problems with
missing requirements and rpmlint is quiet.

APPROVED

Hey Konrad. Whats the status of this package? 
Looks like Paul approved it in comment #32 on 2007-02-09. 

Are you still waiting for a sponsor?
Are you still interested in maintaining this package? 

(In reply to comment #33)
> Hey Konrad. Whats the status of this package? 
> Looks like Paul approved it in comment #32 on 2007-02-09. 
> 
> Are you still waiting for a sponsor?

Yes!
> Are you still interested in maintaining this package? 

Yes!

Well, apply for cvsextras access and someone (i.e. me if nobody beats me to it)
will sponsor you.  Then make your CVS request and get this checked in.

Note: you're at the "Get a Fedora Account" step in
http://fedoraproject.org/wiki/PackageMaintainers/Join

New Package CVS Request
=======================
Package Name: ibmasm
Short Description: This package contains the tools necessary to control the IBM
Advanced System Management Drivers
Owners: konradr
Branches: F9
InitialCC: 
Cvsextras Commits: yes

Shouldn't the Short Description here be: 
"IBM Advanced System Management Drivers" and not the long description from the
package? 

I concur that it definitely should be shorted. The package does not contain
drivers - just the tools for interacting with the specific driver. I think the
below short description should suffice? 


New Package CVS Request
=======================
Package Name: ibmasm
Short Description: Tools for IBM Advanced System Management Drivers
Owners: konradr
Branches: F9
InitialCC: 
Cvsextras Commits: yes

CVS done.  Note that there's no F-9 branch yet, just devel.

Package imported in CVS and built.

Package Change Request
======================
Package Name:  ibmasm
New Branches: F-7 F-8 F-9

We aren't doing F-9 branches yet. They should be around later in the cycle. 
F-8 and F-7 done.