Bug 226452

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-413e1c297803cfa9de0cc4c56f3ac384bff5dc9e

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

New owner of this package is Ondrej Dvoracek <odvorace>

Hey Ondrej: Can you look at addressing the issues in the review in comment #1?
Thanks. 

Hi,
I'm working on it.
Cheers Ondrej

Hey Ondrej, any progress here?

Hi, I'm new maintainer of system-config-bind

I'm working on these issues now, it should be fixed soon...

Everything should be fixed now: system-config-bind-4.0.5-1.fc9

1 and 2 look good. 

3. Is this really 'GPLv2+'? My understanding was that all Red Hat authored
projects must use 'GPLv2' only. Can you make sure this should be "and any later
version" ?

4. Since this is now in the hosted space, can you perhaps add a note about 
how to check out the specific version of the source used? 
ie, 'hg clone http://hg.fedorahosted.org/hg/system-config-bind/' 
Also, there is I think a way to get hosted to have real release tar.gz files you
can point the URL to. Can you investigate that? 

5, 6, 7, 8, 9 and 10 all look good. ;) 

Can you take a look again at 3 and 4 above? 

Thanks for the fixes here... 

ad 3.
Yes, I'm pretty sure it's ok, almost all system-config tools are GPLv2+. I don't
know about any Red Hat policy against GPL >2.

ad 4.
I made a note to https://fedorahosted.org/system-config-bind/wiki/ on how to get
sources and tarballs of older releases. The real tarballs are in distCVS and
source RPMs, I hope that suffice, as I would not be happy having to upload each
release twice :-)

for #3: 

I am not a employee of Red Hat, so I can't show you any specific place, but I
was told to refer you to "CopyrightGuidelines on the intranet". Is that
something you can find? Or does that mean anything to you? 

for #4: 

Take a look at: https://fedorahosted.org/web/faq and see the "How can I publish
archive releases (tgz, zip, etc) for my project?" You should be able to use that
to upload releases and then you can point your Source0: to that url.

3.
Ok, I will update the copyright note and license tag according those guildlines.
Thanks.

4.
Source0 pointing to an URL does not work. It apparently must be local file. In
Fedora CVS, it's solved by special makefile rules and lookaside file cache. To
use hosted space for releases, I'd need to adjust the common makefile, otherwise
the problem with double uploads stays. I understand that hosted releases are
good for external use, but my notes on getting tarball from HG should be
sufficient...

Sorry for the delay here... 

on #3: ok, have you checked in an update on that yet?

on #4: Well, you are the maintainer, so it's up to you. A full Source0 url is
nice for people who are not part of fedora, but I have no idea how many or if
any such users of this package exist. If you could add your note about how to
check out the source to the spec file, that would be just fine. 

> on #3: ok, have you checked in an update on that yet?
Yes.

> on #4: Well, you are the maintainer, so it's up to you. A full Source0 url is
> nice for people who are not part of fedora, but I have no idea how many or if
> any such users of this package exist. If you could add your note about how to
> check out the source to the spec file, that would be just fine. 
I changed my mind, uploaded the tarball to hosted and added that URL to source0.
Hope it'll be useful for someone :)

Excellent. I see no further blockers here, so this package is APPROVED. 

Thanks for all your work on this package. 
Feel free to close this review RAWHIDE whenever you like. 

Ok, thanks for the review. Closing this bug.