Bug 427586 - Review Request: kerneloops - Tool to automatically collect and submit kernel crash signatures
Summary: Review Request: kerneloops - Tool to automatically collect and submit kernel...
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2008-01-04 21:55 UTC by Chuck Ebbert
Modified: 2008-05-17 17:56 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-05-17 17:56:01 UTC
Type: ---
cebbert: fedora_requires_release_note?
j: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)

Description Chuck Ebbert 2008-01-04 21:55:15 UTC
Spec URL: http://people.redhat.com/cebbert/kerneloops.spec
SRPM URL: http://www.kerneloops.org/download/kerneloops-0.9-1.fc8.src.rpm
This package contains the tools to collect kernel crash signatures,
and to submit them to the kerneloops.org website where the kernel
crash signatures get collected and grouped for presentation to the
Linux kernel developers.

Comment 2 Chuck Ebbert 2008-01-05 00:19:57 UTC
I reviewed it as best I could and didn't find any problems.

It is GPL v2, rpmlint only throws one warning about using spaces instead of tabs
in the specfile, it meets packaging and naming standards as far as I can tell etc.

Comment 3 Jason Tibbitts 2008-01-05 04:38:15 UTC
Interestingly, the tarball in this srpm and the one upstream are not the same;
several files differ, though all seem to differ by comments and whitespace.

A few rpmlint complaints:

  kerneloops.src: W: mixed-use-of-spaces-and-tabs (spaces: line 53,
   tab: line 1)
Not a big deal.

  kerneloops.x86_64: E: script-without-shebang
I think this shouldn't be executable; the other files I see there don't seem
to be.

  kerneloops.x86_64: W: non-conffile-in-etc
This is OK.

  kerneloops.x86_64: W: spurious-executable-perm
This shouldn't be executable either.

  kerneloops.x86_64: W: service-default-enabled /etc/rc.d/init.d/kerneloops
Services shouldn't be enabled by default.  This usually means the first entry 
on the chkconfig: line in the initscript should be '-'; I guess Default-Start:
should be either not present, empty, or '-' as well, but I'm not sure which it
should be, or if we even have anything that pays attention to it.

  kerneloops.x86_64: W: incoherent-subsys /etc/rc.d/init.d/kerneloops $prog
This is bogus.

Other issues:

The scriptlets to start and stop the service differ from the recommended ones.
For example, the %preun script won't trigger on package removals, so removing
the package will leave the service still running.  And it looks like a %postun
script was intended (given the dependency for it) but it's not actually in the
spec.  See http://fedoraproject.org/wiki/Packaging/ScriptletSnippets

I've not seen a Makefile call desktop-file-itself, but don't see anything
wrong with the way it's being called so I don't see any reason for the spec to
call it explicitly.

X source files don't match upstream:

* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has some valid complaints 
* final provides and requires are sane:
   config(kerneloops) = 0.9-1.fc9
   kerneloops = 0.9-1.fc9
   config(kerneloops) = 0.9-1.fc9

* %check is present but disabled; according to comments, the test suite is
   broken upstream.  I have not attempted to test this package.
* no shared libraries are added to the regular linker search paths.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* %find_lang used properly to collect translations.
X service management scriptlets are not the recommended ones.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.

Comment 4 Arjan van de Ven 2008-01-05 13:18:23 UTC
Thanks for the quick review;

The file permission thing was a bug in the upstream package; I've fixed that.
I've copied the script templates from your URL into the spec
The tar mismatch was a timelag between when I built the rpms and when I ran 
the tar upload script; will not happen again (script will also have to run an 
rpmbuild ;)

The "service starts by default"... this service really needs to start by 
default, that's the whole point of it.

I've released version 0.10 of the program to fix the upstream package bugs;
http://www.kerneloops.org/download/  has the tar, src.rpm and binary rpm

Comment 5 Jason Tibbitts 2008-01-07 06:21:08 UTC
Looks much better.  A new rpmlint warning has appeared:
  kerneloops.x86_64: W: non-conffile-in-etc /etc/dbus-1/system.d/kerneloops.dbus
Looking at whether other packages have these files marked %config, it seems that
many packages do, although several like NetworkManager, ConsoleKit, cups and gdm
don't.  However, all package I have installed on my system actually call the
files *.conf.  We really don't have any guidelines here as far as I can tell so
honestly I'm not going to complain.

Everything now matches upstream:

The permission issues are fixed.

About enabling the service by default, you could make the argument that the
whole point of httpd is to serve web pages, and yet it's not started by default.
 However, this service isn't network-facing and so I can't really make a
security argument.  And my personal preferences aside, we don't have any
specific guideline regarding the issue, so I won't block on it.

So, it seems that all the complaints I had are either addressed or moot.


Comment 6 Chuck Ebbert 2008-01-08 17:17:47 UTC
What happens next? I don't see kerneloops in the PackageDB yet...

Comment 7 Chuck Ebbert 2008-01-08 17:29:43 UTC
New Package CVS Request
Package Name: kerneloops
Short Description: Tool to automatically collect and submit kernel crash signatures
Owners: cebbert
Branches: F-8
Cvsextras Commits: no

Comment 8 Kevin Fenzi 2008-01-09 00:07:15 UTC
cvs done. 

Any particular reason for the cvsextras commits: no? 

Comment 9 Chuck Ebbert 2008-01-09 00:32:42 UTC
**** Access denied: cebbert is not in ACL for rpms/kerneloops/devel

Isn't the owner allowed to update the package?

Comment 10 Chuck Ebbert 2008-01-09 00:42:38 UTC
(In reply to comment #9)
> **** Access denied: cebbert is not in ACL for rpms/kerneloops/devel
> Isn't the owner allowed to update the package?

I added myself to the committers.

Comment 11 Kevin Fenzi 2008-01-09 00:46:26 UTC
Package acls are generated twice an hour... It sounds like you tried a commit
after I added it, but before the :31 acl generating job ran. 

Try again and it should work fine? 

Comment 12 Dennis Gilmore 2008-01-09 00:48:38 UTC
a job runs at the top of the hour that updates the acls in CVS  you need to 
wait for that to happen

Comment 13 Chuck Ebbert 2008-01-09 01:09:18 UTC
Should fedora_requires_release_note be set now that the package has built?

Comment 14 Kevin Fenzi 2008-01-09 01:17:30 UTC
Yep. Anytime you like. You might also add a note about what kind of info you
would like them to add for the release note. 

Comment 15 Chuck Ebbert 2008-01-09 22:32:55 UTC
This should do for a release note:

This package contains the tools to collect kernel crash signatures,
and to submit them to the kerneloops.org website where the kernel
crash signatures get collected and grouped for presentation to the
Linux kernel developers.

The information will be used to debug kernel crashes and to see how
common they are, allowing kernel developers to concentrate on more
severe and common bugs.

Comment 16 Jason Tibbitts 2008-02-19 19:06:49 UTC
Is there any reason this can't be closed now?

Comment 17 Chuck Ebbert 2008-02-20 01:58:31 UTC
Was the release note done?

Comment 18 Kevin Fenzi 2008-02-20 23:48:58 UTC
in reply to comment #17: 

Not yet I don't think... someone from docs should comment here and set the flag
to + (with a link to where it landed) or - (to say why they didn't think it was
release note worthy. ;( 

While we are waiting, "Any particular reason for the cvsextras commits: no?" 

Comment 19 Jason Tibbitts 2008-02-21 17:10:17 UTC
My understanding is that the release note status is orthogonal to the resolution
of the ticket.  At least that's how we process the CVS queue; if the flag is '?'
we look at it, regardless of whether the ticket is open or now.

Honestly I'm just trying to get my bug list down, because it gets difficult to
track how many reviews I have in flight.

Comment 20 Peter Lemenkov 2008-05-17 17:56:01 UTC
Closing this bug. This package already in Fedora and there is no need to pollute list of Review Requests 
with already reviewed package.

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