[Bug 441570] Review Request: dnx - Distributed Nagios Executor

Sat Aug 23 23:15:37 UTC 2008

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=441570

--- Comment #20 from Kevin Fenzi <kevin at tummy.com>  2008-08-23 19:15:35 EDT ---
Hey Jeff. Sorry for the long delay here... lets get this moving again with a
formal review: 

OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPLv2)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
b8e262dd148f22d000015e638ab75b7e  dnx-0.18.tar.gz
b8e262dd148f22d000015e638ab75b7e  dnx-0.18.tar.gz.orig
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
OK - Package is code or permissible content.
OK - Doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.
see below - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
See below - Package has no duplicate files in %files.
See below - Package doesn't own any directories other packages own.
See below - 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 have dist tag
OK - Should package latest version

Issues:

1. There shouldn't be any need for
rm -rf %{buildroot} >/dev/null 2>&1
at the top of %install... should just be
rm -rf %{buildroot}
(there shouldn't be any output from that ever.

2. rpmlint says:

dnx.src:142: E: files-attr-not-set
dnx.src:143: E: files-attr-not-set
I think this is due to the doc subpackage missing a:
%defattr(-,root,root,-)
line

dnx.x86_64: W: non-standard-uid /var/run/dnx nagios
dnx.x86_64: W: non-standard-gid /var/run/dnx nagios
dnx.x86_64: W: non-standard-uid /var/log/dnx nagios
dnx.x86_64: W: non-standard-gid /var/log/dnx nagios
dnx-server.x86_64: W: non-standard-uid /var/log/dnx nagios
dnx-server.x86_64: W: non-standard-gid /var/log/dnx nagios

Ignore.

dnx.x86_64: W: log-files-without-logrotate /var/log/dnx
dnx-server.x86_64: W: log-files-without-logrotate /var/log/dnx

Need to make a logrotate file and add a Requires: logrotate?

dnx.x86_64: W: no-reload-entry /etc/rc.d/init.d/dnx

Would be nice to have if it makes sense. Not sure if this can be
reloaded though. Can it?

dnx.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/dnx-0.18/README
dnx-server.x86_64: W: wrong-file-end-of-line-encoding
/usr/share/doc/dnx-server-0.18/README

Would be nice to fix with a sed. Check the wiki for recipe.

dnx-doc.x86_64: W: file-not-utf8
/usr/share/doc/dnx-doc-0.18/dnx-doxy-0.18.tar.gz

This could be unpacked and shipped unpacked?

3. Some of the directories here are owned in the base package and the
subpackages
as well. Could you describe why or rethink this? What are the use cases?

doc - used by itself, shouldn't need any others?
server - could be used by itself? or does it need the base package?
base - by itself or not?

This ties in with the users setup... the nagios package (currently required by
this server subpackage) already makes a nagios user, why do that here?
I also see things like the base package having files owned by nagios, but it
has no dependency there or user creation.

It's best to try and make only one package own dirs/files and use dependencies
for subpackages that need those directories or files. Ie, if the server needs
the base package we can remove some of the dual listed dirs in the server
subpackage. Does that make sense?

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