Bug 243187 - Review Request: edac-utils - user space utilities for EDAC subsystem
Review Request: edac-utils - user space utilities for EDAC subsystem
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Package Reviews List
:
Depends On:
Blocks: 244088
  Show dependency treegraph
 
Reported: 2007-06-07 15:08 EDT by Aristeu Rozanski
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-13 10:55:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jarod: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Aristeu Rozanski 2007-06-07 15:08:31 EDT
Spec URL: http://people.redhat.com/arozansk/edac-utils/edac-utils.spec
SRPM URL: http://people.redhat.com/arozansk/edac-utils/edac-utils-0.9-1.src.rpm
Description: edac-utils package contains the userspace utilities to perform EDAC
related operations such as labeling memory module slots and checking for EDAC
reported errors.
Comment 1 Jarod Wilson 2007-06-07 15:31:53 EDT
Suggested changes from initial pass through the spec:

1) Use the %{?dist} tag in the Release: field

2) Typical format for sf.net downloads is to use dl.sf.net, i.e.:
http://dl.sourceforge.net/sourceforge/edac-utils/%{name}-%{version}.tar.bz2

3) Remove the CFLAGS= from the make line, its superfluous, as it gets set and
exported by the %configure macro, and make is picking that up fine.

4) Be consistent with use of spaces or tabs for indentation (see %post and %preun).

5) %preun should probably try stopping the edac service before removing the
initscript (although not having looked at the exact nature of the initscript,
I'm guessing this may be essentially a no-op anyhow).

6) The .so and .h files really should go in an edac-utils-devel sub-package.

7) ldconfig needs to be run in %post and %postun

8) static libs and libtool archives shouldn't be installed

9) _smp_mflags should be prefixed with a ? so it doesn't cause build errors
and/or spew on systems that don't have it defined

10) ideally, rpmlint *should* be silent. I've patched up the spec with changes
for all of the above, but its still complaining a bit about the initscript:
$ rpmlint /build/RPMS/x86_64/edac-utils-*
W: edac-utils service-default-enabled /etc/init.d/edac
E: edac-utils missing-mandatory-lsb-tag Short-Description
W: edac-utils service-default-enabled /etc/init.d/edac
W: edac-utils no-reload-entry /etc/init.d/edac
E: edac-utils subsys-not-used /etc/init.d/edac
W: edac-utils incoherent-init-script-name edac
W: edac-utils-devel no-documentation
W: edac-utils-devel no-dependency-on edac-utils

For the most part, the W: bits are fine, but it'd be good to address the two
E:'s there. Both are relatively simple to fix.

Index: edac-utils.spec
===================================================================
RCS file: /cvs/dist/rpms/edac-utils/devel/edac-utils.spec,v
retrieving revision 1.4
diff -u -p -r1.4 edac-utils.spec
--- edac-utils.spec     6 Jun 2007 20:10:50 -0000       1.4
+++ edac-utils.spec     7 Jun 2007 19:31:05 -0000
@@ -1,12 +1,12 @@
 Name:          edac-utils
 Version:       0.9
-Release:       1
+Release:       1%{?dist}
 Summary:       Userspace helper for kernel EDAC drivers
 
 Group:         System Environment/Base
 License:       GPL
 URL:           http://edac-utils.sourceforge.net/
-Source0:      
http://prdownloads.sourceforge.net/edac-utils/%{name}-%{version}.tar.bz2
+Source0:      
http://dl.sourceforge.net/sourceforge/edac-utils/%{name}-%{version}.tar.bz2
 Patch0:                edac_ctl-remove_driver_loading.patch
 Patch1:                edac_init-fix_messages.patch
 
@@ -23,44 +23,62 @@ of an init script which makes sure EDAC 
 are loaded at system startup, as well as a library and utility
 for reporting current error counts from the EDAC sysfs files.
 
+%package devel
+Summary:       Development files for %{name}
+Group:         Development/Libraries
+
+%description devel
+This package contains the development headers and libraries
+for %{name}.
+
 %prep
 %setup -q
 %patch0 -p1
 %patch1 -p1
 
 %build
-%configure
-make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
+%configure --disable-static
+make %{?_smp_mflags} 
 
 %install
 rm -rf $RPM_BUILD_ROOT
 make install-exec install-data DESTDIR="$RPM_BUILD_ROOT"
+# Remove libtool archive
+rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la
 
 %clean
 rm -rf $RPM_BUILD_ROOT
 
 %post
+/sbin/ldconfig
 if [ $1 = 1 ]; then
-  /sbin/chkconfig --add edac
+       /sbin/chkconfig --add edac
 fi
 
 %preun
 if [ $1 = 0 ]; then
+       /sbin/service edac stop >/dev/null 2>&1 || :
        /sbin/chkconfig --del edac
 fi
 
+%postun -p /sbin/ldconfig
+
 %files 
-%defattr(-,root,root,0755)
+%defattr(-,root,root,-)
 %doc README NEWS ChangeLog DISCLAIMER
 %{_sbindir}/edac-ctl
 %{_bindir}/edac-util
-%{_libdir}/*
+%{_libdir}/*.so.*
 %{_mandir}/*/*
-%{_includedir}/edac.h
 %dir %attr(0755,root,root) %{_sysconfdir}/edac
 %config(noreplace) %{_sysconfdir}/edac/*
 %{_sysconfdir}/init.d/edac
 
+%files devel
+%defattr(-,root,root,-)
+%{_libdir}/*.so
+%{_includedir}/edac.h
+
 %changelog
 * Wed Jun 06 2007 Aristeu Rozanski <arozansk@redhat.com> 0.9-1
 - Updated version to 0.9, separate project now
Comment 2 Jarod Wilson 2007-06-13 14:27:25 EDT
Review of latest version:

http://people.redhat.com/arozansk/edac-utils/edac-utils-0.9-3.fc7.src.rpm

* source files match upstream:
f19e0695c09cf707953e9584a03df1e1b8f3f423814ac5ea9b70aeb082f071a6 
edac-utils-0.9.tar.bz2
f19e0695c09cf707953e9584a03df1e1b8f3f423814ac5ea9b70aeb082f071a6 
../edac-utils-0.9.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is sufficient. (could be enhanced slightly, but meets minimum reqs)
* license field matches the actual license - GPL
* license is open source-compatible (GPL), text is included in package
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (fedora-devel-x86_64).
* package installs properly
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane.
* ldconfig run when shared libraries are installed.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets do the right thing.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in -devel.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app, so no GUI deps.

Package approved, import at will.
Comment 3 Aristeu Rozanski 2007-06-22 15:39:14 EDT
New Package CVS Request
=======================
Package Name: edac-utils
Short Description: userspace utilities for EDAC drivers
Owners: arozansk@redhat.com
Branches: FC-6, FC-7
Comment 4 Kevin Fenzi 2007-06-22 15:55:13 EDT
cvs done.

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