Fedora Merge Review: setroubleshoot http://cvs.fedora.redhat.com/viewcvs/devel/setroubleshoot/ Initial Owner: jdennis
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!
Created attachment 461207 [details] Review for devel package setroubleshoot-3.0.8-1.fc15
Thanks this was great. I have a couple of questions. - Spec written in American English What does that mean? Upstream is a git repository. http://git.fedorahosted.org/git/?p=setroubleshoot.git Not sure how that is supposed to go into the spec file. All other fixes should be in setroubleshoot-3.0.9
(In reply to comment #3) > - Spec written in American English That's just the heading I put rpmlint's capitalization complaint under. If you've fixed that then don't worry about it. > Upstream is a git repository. > > http://git.fedorahosted.org/git/?p=setroubleshoot.git > > Not sure how that is supposed to go into the spec file. What you should do in that case is put comments above the Source0 line that tell people how to build tarballs identical to yours. I would use these, personally: # git clone git://git.fedorahosted.org/git/setroubleshoot.git; cd setroubleshoot # git archive --prefix setroubleshoot-3.0.9/ 46de4d7b86bebb8375f69e78a88fe154c9a97800 > setroubleshoot-3.0.9.tar.gz Then all you have to do is update the second line with the right info when you build a new release tarball.
Fixed in setroubleshoot-3.0.10-1.fc15