Bug 538057 (rhnmd)

Let's go through the spec from top to bottom:

Use %global instead of %define, see 
https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

Use %{_sharedstatedir} instead of %{_var}/lib/, see
http://fedoraproject.org/wiki/Packaging:RPMMacros

Is the license GPLv2 or GPLv2+? Maybe add a doc to clarify?

%{_initrddir} is considered deprecated on Fedora, but still needed on RHEL. You could catch this with a conditional, see
https://fedoraproject.org/wiki/Packaging/DistTag#Conditionals
(but that's not really that important)

Add -p to install in order to preserve timestamps of the files, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

Please don't use excessive wildcards like %{_usr}/sbin/*
because no other maintainer will know what this means when looking at the spec.

The spec would be more legible if
- you moved %preun before %clean and %files
- you used proper text indention. The content of tags like Name, Version, ... usually starts at 17 characters (at least in spec generated with rpmdev-newspec).

Add more docs, at least an AUTHORS file should IMO be there, ChangeLog would be nice too.

Please use a more common format for your changelogs. Bug numbers should be at the end of a changelog entry, e.g.
- remove the dependency of rhnmd on nocpulse-common (#494538)
see https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

BTW: there is a typo: "dependecy" instead of "dependency"


The rpmlint warnings are mostly save to ignore, but
- I disagree about service-default-enabled, see
https://fedoraproject.org/wiki/Packaging/SysVInitScript#Why_don.27t_we....
- I don't understand dangerous-command-in-%pre: When you migrate settings from /home/nocpulse, why remove /var/lib/nocpulse/{bin,var}? This doesn't look related.

> Use %global instead of %define, see 
done

> Use %{_sharedstatedir} instead of %{_var}/lib/, see
I'm not willing to use this macro. It eval differently on different systems. E.g in RHEL it eval to /usr/com

> Is the license GPLv2 or GPLv2+? Maybe add a doc to clarify?
It is GPLv2. Even license text is included in %doc, not sure how you want to clarify it even more.

>%{_initrddir} is considered deprecated on Fedora, but still needed on RHEL.
done
 
> Add -p to install in order to preserve timestamps of the files,
done

> Please don't use excessive wildcards like %{_usr}/sbin/*
In some previous reviews, somebody else encourages me to use wildcards. So what is correct behaviour? 
But I do not care :) Done.

> you moved %preun before %clean and %files
Done

> Add more docs, at least an AUTHORS file should IMO be there, 
Negative. We do not have doc separately for rhnmd, we have documentation for Satellite and when the times come, we can make separate package from it. No intention to put it here due its size.
Authors changes a lot. And we (as Spacewalk team) are not willing to maintains such document file.

> ChangeLog would be nice too.
We have changelog in spec. Guidelines (https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs) do not say anything about separate file. So ... negative.

> Please use a more common format for your changelogs.
> Bug numbers should be at the end of a changelog entry, e.g.
I would not say it is more common (at least outside of Fedora). On fedora-devel-list was discussion on April this year and there was no conclusion about format of changelog (apart from first line). And since our changelog is generated in upstream from commit messages, I can do hardly anything about it, unless I rewrite it or force upstream developers to change their behaviour. :(

> BTW: there is a typo: "dependecy" instead of "dependency"
fixed

> I don't understand dangerous-command-in-%pre: When you migrate settings 
> from /home/nocpulse, why remove /var/lib/nocpulse/{bin,var}? This doesn't
> look related.  
This for user who had previous version of rhnmd. It was crappy package and when you upgrade this two path will remain on your disk with some dynamicaly created links and files, which were previously not present in %files

> I disagree about service-default-enabled, see
Well this package is just special wrapper around sshd, and we behave to it
similar way as to sshd. And sshd is enabled by default to.

Updated SRPM:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd-5.3.5-1.src.rpm
Updated SPEC:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd.spec

Moving to space13.

Thanks Miroslav for attending to the issues that Christoph raised, or explaining why you would not do so. 

The package looks good now, but because it has been over a year with no more review, I note that there is a new version available upstream. I assume it is this version that you want to ship, so please generate a new SRPM based on 5.3.7 and I will approve it if no new issues appear.

Cheers
Rudi


 - = N/A
 / = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [/] Rpmlint output is clean:
$ rpmlint SPECS/rhnmd.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint SRPMS/rhnmd-5.3.5-1.fc14.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint RPMS/noarch/rhnmd-5.3.5-1.fc14.noarch.rpm 
rhnmd.noarch: W: dangling-relative-symlink /usr/sbin/rhnmd sshd
rhnmd.noarch: E: non-readable /var/lib/nocpulse/.ssh/authorized_keys 0600L
rhnmd.noarch: W: hidden-file-or-dir /var/lib/nocpulse/.ssh
rhnmd.noarch: E: non-standard-dir-perm /var/lib/nocpulse/.ssh 0700L
rhnmd.noarch: W: hidden-file-or-dir /var/lib/nocpulse/.ssh
rhnmd.noarch: W: no-manual-page-for-binary rhnmd
rhnmd.noarch: W: dangerous-command-in-%pre rm
rhnmd.noarch: W: service-default-enabled /etc/rc.d/init.d/rhnmd
rhnmd.noarch: W: service-default-enabled /etc/rc.d/init.d/rhnmd
1 packages and 0 specfiles checked; 2 errors, 7 warnings.

* all these issues have already been discussed and explained. It would be nice to have at least a basic manpage in there, even if it referred to other Spacewalk documentation for more detail, but I don't consider this a blocker.
 
 [/] Package is named according to the Package Naming Guidelines.
 [/] Spec file name must match the base package %{name}, in the format
%{name}.spec.
 [/] Package meets the Packaging Guidelines including the Language specific
items
 [/] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [/] License field in the package spec file matches the actual license.
     License type: GPLv2
 [/] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [/] Spec file is legible and written in American English.
 [/] Sources used to build the package matches the upstream source, as provided
in the spec URL.
  (md5sum 87d7f27b7ddcd1c1140192fb6ac746c0)
 [/] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested: http://koji.fedoraproject.org/koji/taskinfo?taskID=2896478
 [/] Package is not known to require ExcludeArch
 [/] All build dependencies are listed in BuildRequires, except for any that
are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly (with the %find_lang macro)
 [-] ldconfig called in %post and %postun if required.
 [/] Package does not bundle copies of system libraries
 [/] Package is not relocatable.
 [/] Package must own all directories that it creates.
 [/] Package does not contain duplicates in %files.
 [/] Permissions on files are set properly
 [/] %files section includes a %defattr(...) line
 [/] Package consistently uses macros.
 [-] Large documentation files are in a -doc subpackage, if required.
 [/] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -static subpackage, if present.
 [-] Development .so files in -devel subpackage, if present.
 [-] -devel packages require base package with full versioning.
 [/] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [/] Package does not own files or directories owned by other packages.
 [/] Filenames are valid UTF-8

There was only small change:
Spec:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd.spec

src.rpm:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd-5.3.7-1.fc14.src.rpm

Thanks: looks good except for:

rhnmd.spec:19: W: unversioned-explicit-obsoletes rhnmd.i386
rhnmd.spec:20: W: unversioned-explicit-obsoletes rhnmd.x86_64
rhnmd.spec:19: W: mixed-use-of-spaces-and-tabs (spaces: line 7, tab: line 19)

rhnmd.noarch: W: obsolete-not-provided rhnmd.i386
rhnmd.noarch: W: obsolete-not-provided rhnmd.x86_64

If you're going to obsolete the arch-specific versions of the packages, you should also 

1. specify the last version of the arch-specific packages
2. add corresponding "Provides:" for the arch-specific packages

On the other hand, because this package is new to Fedora, I question whether you need to obsolete these at all -- how many Fedora machines are likely to have the arch-specific versions of the packages installed on them?

(In reply to comment #6)
> 
> On the other hand, because this package is new to Fedora, I question whether
> you need to obsolete these at all -- how many Fedora machines are likely to
> have the arch-specific versions of the packages installed on them?

The package was shipped by Spacewalk project for Fedora in the past. So you may well have installations of old non-Fedora package and now upgrade with package that made it to Fedora.

Fixed in spacewalk.git commits:
7fda56e92719610c8e0fc14573a5111ada66e438
648558e2835ec6cbe31b12bbf44f1c7297914e81

Spec:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd.spec

src.rpm:
http://miroslav.suchy.cz/fedora/rhnmd/rhnmd-5.3.8-1.fc14.src.rpm

Thanks Miroslav -- all looks good now. Please go ahead and make your SCM request

ACCEPT

New Package CVS Request
=======================
Package Name: rhnmd
Short Description: Red Hat Network Monitoring Daemon
Owners: msuchy
Branches: F-14, F-15
InitialCC:

This ticket is not assigned to anyone.  It should be assigned to the reviewer.

Please fix and re-raise the fedora-cvs flag.

Git done (by process-git-requests).

rhnmd-5.3.8-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/rhnmd-5.3.8-1.fc15

rhnmd-5.3.8-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/rhnmd-5.3.8-1.fc14

rhnmd-5.3.8-1.fc14 has been pushed to the Fedora 14 stable repository.

rhnmd-5.3.8-1.fc15 has been pushed to the Fedora 15 stable repository.