[Bug 226410] Merge Review: setroubleshoot

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=226410

Garrett Holmstrom <gholms at fedoraproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |gholms at fedoraproject.org
         AssignedTo|nobody at fedoraproject.org    |gholms at fedoraproject.org
               Flag|                            |fedora-review?

--- Comment #1 from Garrett Holmstrom <gholms at fedoraproject.org> 2010-11-17 23:48:08 EST ---
The most important issues with this package are outlined below.  I will also
attach a full review shortly.

- License files installed when any subpackage combination is installed

Unless the other packages pull in the docs subpackage you will need to put the
"COPYING" file in another subpackage.  (setroubleshoot-server, perhaps?)

- Spec written in American English

rpmlint nitpick:  just change "gui" to "GUI" in the %description

- Sources match upstream unless altered to fix permissibility issues

Where are upstream's tarballs?

- File permissions are sane
  /usr/lib64/python2.7/site-packages/setroubleshoot/default_encoding_utf8.so
0775
  /usr/lib64/python2.7/site-packages/setroubleshoot/sesearch/_sesearch.so 0775

I assume these should actually be 0755?

- GUI app installs .desktop file w/ desktop-file-install or has justification

Please desktop-file-install instead of update-desktop-database or add a comment
that justifies otherwise.

- Scriptlets are sane

What is the purpose of chowning %pkgdatabase in %post?

- Changelog in prescribed format

The entry for 3.0.6-1 seems to be mangled somehow.

- Requires correct, justified where necessary

Please depend on desktop-file-utils instead of /usr/bin/update-desktop-database
for the %post and %postun scripts.

Why do you Require libnotify when the generated packages already require
libnotify.so.4?

You can probably drop the fc7/8/11 conditionals now.

- Config files marked with %config
  /etc/audisp/plugins.d/sedispatch.conf
  /etc/xdg/autostart/sealertauto.desktop

I think the first needs to be %config(noreplace), while the second should get
just %config.

- %config files marked noreplace or justified
  /etc/dbus-1/system.d/org.fedoraproject.Setroubleshootd.conf
  /etc/dbus-1/system.d/org.fedoraproject.SetroubleshootFixit.conf
  /etc/setroubleshoot/setroubleshoot.cfg
  /usr/share/polkit-1/actions/org.fedoraproject.setroubleshootfixit.policy

While it's understandable to have PolicyKit and dbus configs without noreplace,
the spec file should contain commentary about it.

- No %config files under /usr
  /usr/share/polkit-1/actions/org.fedoraproject.setroubleshootfixit.policy

Unfortunately this is mandatory.  If users aren't supposed to edit this then I
would just remove the %config tag.

I hope that helps!

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.