Bug 193059
Summary: | Review Request: ibmasm | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Konrad Rzeszutek <konradr> | ||||
Component: | Package Review | Assignee: | Paul F. Johnson <paul> | ||||
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | amax, cweyl, j, kevin | ||||
Target Milestone: | --- | Flags: | j:
fedora-review+
kevin: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2007-12-21 19:50:34 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 163779, 194539 | ||||||
Attachments: |
|
Description
Konrad Rzeszutek
2006-05-24 20:20:22 UTC
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. |