Bug 243187 - Review Request: edac-utils - user space utilities for EDAC subsystem
Summary: Review Request: edac-utils - user space utilities for EDAC subsystem
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Jarod Wilson
QA Contact: Fedora Package Reviews List
Depends On:
Blocks: 244088
TreeView+ depends on / blocked
Reported: 2007-06-07 19:08 UTC by Aristeu Rozanski
Modified: 2007-11-30 22:12 UTC (History)
0 users

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

Attachments (Terms of Use)

Description Aristeu Rozanski 2007-06-07 19:08:31 UTC
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 19:31:53 UTC
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.:

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/
 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}.
 %setup -q
 %patch0 -p1
 %patch1 -p1
-make %{_smp_mflags} CFLAGS="$RPM_OPT_FLAGS"
+%configure --disable-static
+make %{?_smp_mflags} 
 make install-exec install-data DESTDIR="$RPM_BUILD_ROOT"
+# Remove libtool archive
+rm -f $RPM_BUILD_ROOT/%{_libdir}/*.la
 if [ $1 = 1 ]; then
-  /sbin/chkconfig --add edac
+       /sbin/chkconfig --add edac
 if [ $1 = 0 ]; then
+       /sbin/service edac stop >/dev/null 2>&1 || :
        /sbin/chkconfig --del edac
+%postun -p /sbin/ldconfig
 %dir %attr(0755,root,root) %{_sysconfdir}/edac
 %config(noreplace) %{_sysconfdir}/edac/*
+%files devel
 * 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 18:27:25 UTC
Review of latest version:


* source files match upstream:
* 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 19:39:14 UTC
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 19:55:13 UTC
cvs done.

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