Bug 193059 - Review Request: ibmasm
Review Request: ibmasm
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 194539
  Show dependency treegraph
 
Reported: 2006-05-24 16:20 EDT by Konrad Rzeszutek
Modified: 2008-01-22 18:33 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-12-21 14:50:34 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Dmesg output after installing ibmasm under FC5 - SELinux label error (562 bytes, text/plain)
2006-06-16 11:06 EDT, Konrad Rzeszutek
no flags Details

  None (edit)
Description Konrad Rzeszutek 2006-05-24 16:20:22 EDT
Spec URL: http://www.darnok.org/ibmasm/3.0-4/SRPMS/ibmasm.spec
SRPM URL: http://www.darnok.org/ibmasm/3.0-4/SRPMS/ibmasm-3.0-4.src.rpm
Description: This package contains the tools necessary to utilize the IBM Advanced
System Management driver.
Comment 1 Chris Weyl 2006-05-26 00:49:09 EDT
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?
Comment 2 Konrad Rzeszutek 2006-05-26 10:11:28 EDT
(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.
Comment 3 Chris Weyl 2006-05-26 11:36:06 EDT
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....
Comment 4 Konrad Rzeszutek 2006-06-16 10:48:51 EDT
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
Comment 5 Konrad Rzeszutek 2006-06-16 11:06:23 EDT
Created attachment 131048 [details]
Dmesg output after installing ibmasm under FC5 - SELinux label error

SELinux does not have ibmasmfs labelled.
Comment 6 Konrad Rzeszutek 2006-06-16 11:24:09 EDT
FYI: I tested the ibmasm-3.0-5 on a fresh FC5 install with selinux=0 and the
package works.
Comment 7 Konrad Rzeszutek 2006-07-05 14:43:45 EDT
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?
Comment 8 Till Maas 2006-07-05 18:06:43 EDT
- 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.
Comment 9 Konrad Rzeszutek 2006-07-06 14:47:55 EDT
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.
Comment 10 Konrad Rzeszutek 2006-07-06 14:49:01 EDT
err, correction (wrong version):

SRPMS URL: http://prdownloads.sourceforge.net/ibmasm/ibmasm-3.0-7.src.rpm?download
Comment 11 Jason Tibbitts 2006-07-08 23:15:13 EDT
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.
Comment 12 Paul F. Johnson 2006-07-28 17:02:13 EDT
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.
Comment 13 Paul F. Johnson 2006-07-28 17:18:52 EDT
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.
Comment 14 Chris Weyl 2006-07-28 17:58:32 EDT
(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.)
Comment 15 Konrad Rzeszutek 2006-07-31 12:01:04 EDT
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.
Comment 16 Paul F. Johnson 2006-07-31 12:47:03 EDT
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)?
Comment 17 Ville Skyttä 2006-07-31 14:39:10 EDT
(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
Comment 18 Paul F. Johnson 2006-08-10 17:43:02 EDT
#15 : any sign of the update?
Comment 19 Konrad Rzeszutek 2006-08-11 12:22:42 EDT
(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.
Comment 20 Konrad Rzeszutek 2006-08-14 18:56:38 EDT
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?).
Comment 21 Paul F. Johnson 2006-08-15 02:27:05 EDT
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.
Comment 22 Konrad Rzeszutek 2006-08-15 11:41:23 EDT
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
Comment 23 Paul F. Johnson 2006-09-02 20:26:47 EDT
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)
Comment 24 Konrad Rzeszutek 2006-09-05 14:04:28 EDT
(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


Comment 25 Paul F. Johnson 2006-09-10 11:12:58 EDT
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.

Comment 26 Konrad Rzeszutek 2006-09-18 10:56:25 EDT
Paul,
A week passed so fast for me. I will get this done by Wednesday.
Comment 27 Konrad Rzeszutek 2006-09-19 18:16:49 EDT
>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
Comment 28 Paul F. Johnson 2006-11-03 03:14:02 EST
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.


Comment 29 Konrad Rzeszutek 2006-11-20 14:38:37 EST
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.
Comment 30 Paul F. Johnson 2006-12-27 17:00:14 EST
Ping - has anything happened with this?
Comment 31 Konrad Rzeszutek 2007-01-04 16:17:30 EST
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?
Comment 32 Paul F. Johnson 2007-02-09 04:19:49 EST
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
Comment 33 Kevin Fenzi 2007-06-01 00:46:50 EDT
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? 

Comment 34 Konrad Rzeszutek 2007-06-01 10:59:27 EDT
(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!
Comment 35 Jason Tibbitts 2007-06-01 12:17:45 EDT
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
Comment 36 Konrad Rzeszutek 2007-10-31 13:47:56 EDT
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
Comment 37 Kevin Fenzi 2007-11-01 13:11:14 EDT
Shouldn't the Short Description here be: 
"IBM Advanced System Management Drivers" and not the long description from the
package? 

Comment 38 Konrad Rzeszutek 2007-11-01 19:37:08 EDT
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
Comment 39 Jason Tibbitts 2007-11-01 21:57:14 EDT
CVS done.  Note that there's no F-9 branch yet, just devel.
Comment 40 Konrad Rzeszutek 2007-12-21 14:50:34 EST
Package imported in CVS and built.
Comment 41 Konrad Rzeszutek 2008-01-22 17:18:04 EST
Package Change Request
======================
Package Name:  ibmasm
New Branches: F-7 F-8 F-9
Comment 42 Kevin Fenzi 2008-01-22 18:33:38 EST
We aren't doing F-9 branches yet. They should be around later in the cycle. 
F-8 and F-7 done. 

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