Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: system-config-bind
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226452
kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|nobody@fedoraproject.org |kevin@tummy.com Flag| |fedora-review?
------- Additional Comments From kevin@tummy.com 2007-03-13 23:44 EST ------- OK - Package meets naming and packaging guidelines OK - Spec file matches base package name. See below - Spec has consistant macro usage. OK - Meets Packaging Guidelines. See below - License See below - License field in spec matches See below - License file included in package OK - Spec in American English OK - Spec is legible. See below - Sources match upstream md5sum: OK - BuildRequires correct OK - Spec handles locales/find_lang See below - Package has %defattr and permissions on files is good. OK - Package has a correct %clean section. See below - Package has correct buildroot OK - Package is code or permissible content. OK - Packages %doc files don't affect runtime.
OK - Package is a GUI app and has a .desktop file
OK - Package compiles and builds on at least one arch. OK - Package has no duplicate files in %files. OK - Package doesn't own any directories other packages own. OK - Package owns all the directories it creates. See below - No rpmlint output. See below - final provides and requires are sane
SHOULD Items:
OK - Should build in mock. OK - Should build on all supported archs OK - Should function as described. See below - Should have sane scriptlets. OK - Should have subpackages require base package with fully versioned depend. OK - Should have dist tag OK - Should package latest version 10 outstanding bugs - check for outstanding bugs on package.
Issues:
1. You use $RPM_BUILD_ROOT and %{buildroot}. Pick one style and use that.
2. The URL seems to go to a 404 page: URL: http://people.redhat.com/~jvdias/system-config-bind
3. The spec says the license is GPL, but there's no mention of the license in the contents of the tar.gz. Perhaps a COPYING file or a README explaining that it's released under the GPL.
4. Since redhat/fedora is upstream for this package, can you add a note as suggested in: http://www.fedoraproject.org/wiki/Packaging/SourceURL#head-413e1c297803cfa9d...
5. You have a desktop file, but aren't installing it with desktop-file-install. Perhaps take a look at: http://www.fedoraproject.org/wiki/PackagingDrafts/DesktopFiles
6. Is there a reason that every file in this package is in the "bind" group? This results in 361 "non-standard-gid" warnings from rpmlint.
7. The python >= 2.2 Requires shouldn't be needed anymore.
8. %define debug_package %{nil} shouldn't be needed, this is a noarch package.
9. Is all the removing and linking in post and triggerun needed anymore? How long ago was that version? Looks like aug 2005... perhaps we can remove it now?
10. rpmlint says:
a) non-standard-gid (361 times)
Why does this need to be gid bind? Doesn't it run as root?
b) E: system-config-bind invalid-desktopfile /tmp/system-config-bind-4.0.2-3.fc7.noarch.rpm.8818/usr/s hare/applications/system-config-bind.desktop
Suggest: Install desktop file correctly.
c) E: system-config-bind no-cleaning-of-buildroot %install
Suggest: rm -rf $RPM_BUILD_ROOT at the top of install?
d) E: system-config-bind non-standard-executable-perm /usr/share/system-config-bind/BIND.py 0754 (repeated for every file in that directory)
Suggest: Permissions on those should be 0755 if they are scripts to be executed, or more likey if they are just modules being included they should be 0644.
e) E: system-config-bind obsolete-not-provided bindconf E: system-config-bind obsolete-not-provided redhat-config-bind
Suggest: Do we need to keep these around still?
f) E: system-config-bind script-without-shebang /usr/share/system-config-bind/EditDialog.py E: system-config-bind script-without-shebang /usr/share/system-config-bind/NewZone.py E: system-config-bind script-without-shebang /usr/share/system-config-bind/system-config-bind.glade E: system-config-bind script-without-shebang /usr/share/system-config-bind/View.py
Suggest: perms should be 0644?
g) W: system-config-bind dangerous-command-in-%post rm W: system-config-bind dangerous-command-in-%trigger rm
Suggest: can we just do away with that post and trigger stuff?
h) W: system-config-bind macro-in-%changelog version
Suggest: Change the %version to %%version int he changelog.
i) W: system-config-bind mixed-use-of-spaces-and-tabs (spaces: line 40, tab: line 1)
Suggest: minor. Could change to all spaces.
j) W: system-config-bind no-dependency-on usermode W: system-config-bind no-dependency-on usermode
Suggest: does there need to be a dependency on usermode here?
k) W: system-config-bind non-conffile-in-etc /etc/pam.d/bindconf W: system-config-bind non-conffile-in-etc /etc/pam.d/system-config-bind W: system-config-bind non-conffile-in-etc /etc/security/console.apps/bindconf W: system-config-bind non-conffile-in-etc /etc/security/console.apps/system-config-bind
Suggest: Should any of those be marked %config? or %config(noreplace)?
l) W: system-config-bind rpm-buildroot-usage %build rm -rf $RPM_BUILD_ROOT
Suggest: get rid of the rm -rf $RPM_BUILD_ROOT at the top of build.
m) W: system-config-bind summary-ended-with-dot The Red Hat BIND DNS Configuration Tool.
Suggest: remove . at end of summary.
n) W: system-config-bind symlink-should-be-relative /usr/share/locale/ar/LC_MESSAGES/bindconf.mo /usr/ share/locale/ar/LC_MESSAGES/system-config-bind.mo
Suggest: Make those relative symlinks (works better in chroot type setups).
11. Might check the outstanding bugs and see if any can be cleaned up as part of this review. In particular you might fix the typos mentioned in 232054