Bug 458204

Summary: Review Request: coredumper - Library to help applications create core dumps
Product: [Fedora] Fedora Reporter: Rakesh Pandit <rpandit>
Component: Package ReviewAssignee: Debarshi Ray <debarshir>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: debarshir, fedora-package-review, notting, rpandit
Target Milestone: ---Flags: debarshir: 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: 2008-09-06 07:57:27 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: 461340, 461341, 461342    

Description Rakesh Pandit 2008-08-06 22:50:13 UTC
Description:
The coredumper library can be compiled into applications to create
core dumps of the running program -- without terminating. It supports
both single- and multi-threaded core dumps, even if the kernel does
not natively support multi-threaded core files.

SPEC: http://rakesh.fedorapeople.org/spec/coredumper.spec
SRPM: http://rakesh.fedorapeople.org/srpm/coredumper-1.2.1-1.fc9.src.rpm

Comment 1 Debarshi Ray 2008-08-24 14:04:47 UTC
MUST Items: 

OK - rpmlint is clean
OK - follows Naming Guidelines
OK - spec file is named as %{name}.spec

xx - package does not meet Packaging Guidelines
    + Why does the -devel subpackage have 'Requires: automake'? The sample Spec
      included in the upstream tarball (packages/rpm/rpm.spec) does not have it
      too.
    + Please add a %check stanza as:
      %check
      make check
      According to the README, a 'BuildRequires: binutils gdb' will be needed
      to run the tests.
    + Moving the library headers from %{_includedir}/google to
      %{_includedir}/%{name} will break the builds of programs which contain
      '#include <google/coredumper.h>'. If similar Google projects are also
      using %{_includedir}/google, then you might not need to move the files.
      Otherwise, if you want to move them there are a few options:
      - Provide a README.Fedora in %doc as mentioned in
        https://fedoraproject.org/wiki/Packaging/Guidelines#Summary_and_description
        to document this.
      - Provide a pkgconfig file which has 'Cflags: -I${includedir}/coredumper'
      - Initiate a discussion on fedora-packaging
      - It is better to pass --includedir=DIR to %configure instead of using
        the mkdir & mv dance to change the location of the headers.
    + No need to 'find $RPM_BUILD_ROOT -type f -name "*.a" -delete' in
      %install since --enable-static=no was passed to %configure.
    + You could consider shipping ChangeLog under %doc, and putting README in
      the -devel subpackage since it talks about the examples and manual pages.
      README also mentions 'src/coredump_unittest.c' so it might be a good idea
      to ship this single file under %doc in the -devel subpackage.

OK - license meets Licensing Guidelines
OK - License field meets actual license
OK - upstream license file included in %doc
OK - spec file uses American English
OK - spec file is legible
OK - sources match upstream sources
OK - package builds successfully

xx - ExcludeArch is needed
     This library has been tested only on %{ix86} x86_64 and arm (see README),
     and it has been specifically mentioned that it does not work on ppc (see
     ChangeLog). You MUST ExcludeArch atleast ia64, ppc and ppc64. Work is on
     to port Fedora to arm and sparc, so you might want to exclude them as
     well. Once the package is approved please file a bug each for ia64, ppc
     and ppc64, and make them block the following trackers respectively:
     https://bugzilla.redhat.com/show_bug.cgi?id=FE-ExcludeArch-ia64
     https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=FE-ExcludeArch-ppc
     https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=FE-ExcludeArch-ppc64
     Also add a comment in the Spec above the ExcludeArch line mentioning the
     bug numbers for ia64, ppc and ppc64.

OK - build dependencies correctly listed
OK - no locales
OK - %post and %postun invoke ldconfig
OK - package is not relocatable

xx - file and directory ownership
     + The %files stanza for the -devel subpackage is broken. It should be
       fixed to have '%dir %{_includedir}/%{name}'. Otherwise the
       %{_includedir}/coredumper directory will not be owned by the package:
       [rishi@ginger SPECS]$ rpm -qf /usr/include/coredumper
       file /usr/include/coredumper is not owned by any package
       [rishi@ginger SPECS]$
     + It might be better not to mention the compression suffix (ie. .gz) for
       the manual pages.

OK - no duplicates in %file
OK - file permissions set properly
OK - %clean present
OK - macros used consistently
OK - contains code and permissable content
OK - -doc is not needed
OK - contents of %doc does not affect the runtime
OK - header files in -devel
OK - no static libraries
OK - no pkgconfig files
OK - library files without suffix in -devel
OK - -devel requires base package
OK - no libtool archives
OK - %{name}.desktop file not needed
OK - does not own files or directories owned by other packages
OK - buildroot correctly prepped
OK - all file names valid UTF-8

SHOULD Items:

OK - upstream provides license text
xx - no translations for description and summary
OK - package builds in mock successfully
OK - package builds on all supported architectures

xx - package functions as expected
     + Only works on %{ix86}, x86_64 and arm. Other architectures should be,
       especially ia64, ppc and ppc64, should be excluded.

OK - scriptlets are sane
OK - subpackages other than -devel are not needed
OK - no pkgconfig files
OK - no file dependencies

Comment 2 Rakesh Pandit 2008-08-24 18:14:46 UTC
The file %{_includedir}/linux/dirent.h was dropped from kernel-headers on rawhide. So, package fails on koji. So, this package wouldn't build on rawhide. 

I have fixed other issues.
SPEC: http://rakesh.fedorapeople.org/spec/coredumper.spec
SRPM: http://rakesh.fedorapeople.org/srpm/coredumper-1.2.1-3.fc9.src.rpm

For linux/dirent.h I will contact upstream.

Comment 4 Rakesh Pandit 2008-08-31 18:53:49 UTC
http://code.google.com/p/google-coredumper/source/detail?r=31 <-- Fixed upstream

I have applied the same upstream patch to package(removing old patch). As soon as next version is released patch will be removed.

Build successfully
http://koji.fedoraproject.org/koji/taskinfo?taskID=797003

SPEC: http://rakesh.fedorapeople.org/spec/coredumper.spec 
SRPM: http://rakesh.fedorapeople.org/srpm/coredumper-1.2.1-5.fc9.src.rpm

Comment 5 Debarshi Ray 2008-09-01 19:19:10 UTC
README also mentions 'src/coredump_unittest.c' so did you consider shipping this single file under %doc in the -devel subpackage?

Also add alpha, arm, s390 and sparc to the list of excluded architectures.

Everything else looks fine. Don't forget to file a bug for each of the excluded architectures and make them block on the individual architecture trackers. The Spec should have the bug numbers in a comment.

+---------------------------------+
| This package is APPROVED by me. |
+---------------------------------+

Comment 6 Rakesh Pandit 2008-09-02 04:57:16 UTC
Thanks, I will incorporate all changes before moving on. In the meantime I have requested for cvs.

New Package CVS Request
=======================
Package Name: coredumper
Short Description: Library to help applications create core dumps.
Owners: rakesh
Branches: F-8 F-9
InitialCC: rakesh
Cvsextras Commits: yes

Comment 7 Kevin Fenzi 2008-09-03 20:26:38 UTC
cvs done.

Comment 8 Fedora Update System 2008-09-06 07:49:42 UTC
coredumper-1.2.1-6.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/coredumper-1.2.1-6.fc8

Comment 9 Fedora Update System 2008-09-06 07:51:14 UTC
coredumper-1.2.1-6.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/coredumper-1.2.1-6.fc9

Comment 10 Fedora Update System 2008-10-01 06:34:26 UTC
coredumper-1.2.1-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 11 Fedora Update System 2008-10-01 06:42:40 UTC
coredumper-1.2.1-6.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.