Bug 487392 - Review Request: abrt - Automatic bug detection and reporting tool
Review Request: abrt - Automatic bug detection and reporting tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dan Horák
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-25 14:46 EST by Jiri Moskovcak
Modified: 2015-02-01 17:48 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-04 04:26:57 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dan: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Fixed crash-catcher package (362.92 KB, application/octet-stream)
2009-02-26 13:55 EST, Adam Williamson
no flags Details

  None (edit)
Description Jiri Moskovcak 2009-02-25 14:46:22 EST
Spec URL: http://people.fedoraproject.org/~jmoskovc/crash-catcher.spec
SRPM URL: http://people.fedoraproject.org/~jmoskovc/crash-catcher-0.0.1-2.fc10.src.rpm
Description: CrashCatcher is a tool to help users to detect defects in applications and to create a bug report with all informations needed by maintainer to fix it. It uses plugin system to extend its functionality.
Comment 1 Dan Horák 2009-02-26 03:18:21 EST
formal review is here, see the notes below:

BAD	source files match upstream:
OK	package meets naming and versioning guidelines.
BAD	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (GPLv2+). License text included in package.
OK	latest version is being packaged.
OK*	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
BAD	rpmlint is silent.
OK	final provides and requires look sane.
N/A	%check is present and all tests pass.
OK	shared libraries are added to the regular linker search paths, correct scriptlets exists
BAD	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
Ok	file permissions are appropriate.
BAD	scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK*	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
BAD	not a GUI app

- full URL to source archive is missing
- no need to specify License in all sub-packages, it is inherited from the main package
- parallel make is not used (use make %{?_smp_mflags})
- when you decide to create a -devel subpackage, then you must move the libraries from the main package to a -libs sub-package to be multilib compliant
- use %{_initddir} instead of /etc/rc.d/init.d
- no need to specify BR: glib2-devel, it is resolved automatically via gtkmm24-devel

- rpmlint complains a bit
crash-catcher.x86_64: W: no-documentation
 => mark COPYING and README as %doc

crash-catcher.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libUtils.so
crash-catcher.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libMiddleWare.so
 => you probably want a devel subpackage

crash-catcher.x86_64: E: executable-marked-as-config-file /etc/rc.d/init.d/crash-catcher
crash-catcher.x86_64: W: conffile-without-noreplace-flag /etc/rc.d/init.d/crash-catcher
 => remove %config from the initsctript

crash-catcher.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/crash-catcher ['/usr/lib64']
crash-catcher.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libMiddleWare.so.0.0.1 ['/usr/lib64']
crash-catcher-addon-ccpp.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/hookCCpp ['/usr/lib64']
 => http://fedoraproject.org/wiki/Packaging:Guidelines#Removing_Rpath

crash-catcher-gui.x86_64: E: non-executable-script /usr/share/crash-catcher/CCMainWindow.py 0644
crash-catcher-gui.x86_64: E: non-executable-script /usr/share/crash-catcher/CCDBusBackend.py 0644
 => drop the shebang lines from the scrips, they are not intended to be run on their own

crash-catcher-gui.x86_64: E: script-without-shebang /usr/bin/cc-gui
 => add it
 
crash-catcher-addon-ccpp.x86_64: W: no-documentation
crash-catcher-applet.x86_64: W: no-documentation
crash-catcher-gui.x86_64: W: no-documentation
crash-catcher-plugin-logger.x86_64: W: no-documentation
crash-catcher-plugin-mailx.x86_64: W: no-documentation
crash-catcher-plugin-sqlite3.x86_64: W: no-documentation
 => can be ignored now

crash-catcher-plugin-mailx.x86_64: E: description-line-too-long
 => fix

crash-catcher-addon-ccpp.x86_64: W: devel-file-in-non-devel-package /usr/lib64/crash-catcher/libCCpp.so
crash-catcher-plugin-logger.x86_64: W: devel-file-in-non-devel-package /usr/lib64/crash-catcher/libLogger.so
crash-catcher-plugin-mailx.x86_64: W: devel-file-in-non-devel-package /usr/lib64/crash-catcher/libMailx.so
crash-catcher-plugin-sqlite3.x86_64: W: devel-file-in-non-devel-package /usr/lib64/crash-catcher/libSQLite3.so
 => link plugins with "-avoid-version" in LDFLAGS, then it is ok to have *.so in a non-devel package

- unowned directories:
    %{_sysconfdir}/%{name}
    %{_sysconfdir}/%{name}/plugins
    %{_libdir}/%{name}
    %{_datadir}/%{name}
  first 3 should be owned by the main package, the last one by the -gui sub-package
- the preun scriptlet controls rarpd
- plugins are usually linked with "-avoid-version" in LDFLAGS
- desktop file is missing for the gui
Comment 2 Adam Williamson 2009-02-26 13:55:04 EST
Here's an updated .src.rpm with fixes for all issues identified by the review. The code fixes are implemented as a patch, which obviously the crash-catcher guys should apply to the code in trac rather than keeping as a patch.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 3 Adam Williamson 2009-02-26 13:55:43 EST
Created attachment 333368 [details]
Fixed crash-catcher package
Comment 4 Adam Williamson 2009-02-26 13:56:48 EST
Oh, except obviously I couldn't fix the tarball location as I'm not a crash-catcher developer :) You guys need to put a download section on the site and put the tarball there.
Comment 5 Adam Williamson 2009-02-26 14:06:42 EST
sorry, one more thing - remove 'autoreconf' from the spec once the patch is applied upstream, it's only needed while the patch is touching a Makefile.am.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 6 Dan Horák 2009-03-02 10:03:29 EST
few issues are still remaining
- %{_libdir}/%{name} is unowned - it should belong to the main package
- desktop file for the gui doesn't exist
Comment 7 Jiri Moskovcak 2009-03-02 16:01:51 EST
Ok, latest update, fixed desktop file, %{_libdir}/%{name} is owned by main package.
http://jmoskovc.fedorapeople.org/crash-catcher-0.0.1-10.fc10.src.rpm
http://jmoskovc.fedorapeople.org/crash-catcher.spec
Comment 8 Adam Williamson 2009-03-02 18:31:34 EST
sorry, somehow I missed those two.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 9 Dan Horák 2009-03-03 03:09:02 EST
another round
- desktop-file-utils is missing as BR
- the main package must use "%dir %{_libdir}/%{name}" in the %files section instead of %{_libdir}/%{name} only
- you should replace all occurrences of the string "crash-catcher" with %{name}, it will make it easier to change the name of the package
- the package must use a released source archive or follow the guideline for pre-release packages
Comment 11 Dan Horák 2009-03-03 09:10:04 EST
resolved issue
- source archive available to download and is the same as included in the srpm
    d026802acb81aa2ec26154fd09361b1c30eb70c9  crash-catcher-0.0.1.tar.gz

remaining issues
- the crash-catcher service is on by default
- don't use "--vendor fedora" when installing the desktop file and use only %{_datadir}/applications/%{name}.desktop in %files section for the gui subpackage
- remove "%{_libdir}/%{name}" from %files section of the libs subpackage, because then the directory and all plugins are incorrectly owned by the libs subpackage
Comment 13 Dan Horák 2009-03-03 13:31:41 EST
All issues are fixed, this package is APPROVED.
Comment 14 Jiri Moskovcak 2009-03-03 13:45:38 EST
New Package CVS Request
=======================
Package Name: abrt
Short Description: Automatic bug detection and reporting tool
Owners: jmoskovc@redhat.com zprikryl@redhat.com
Branches: F-10 F-11

#abrt is a new name for crash-catcher, since it's already a trademark..
Comment 15 Kevin Fenzi 2009-03-03 15:32:19 EST
Please use FAS names in Owners field.

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