[Bug 524105] Review Request: fence-virt - Modular virtual machine fencing daemon

[bugzilla at redhat.com]

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





--- Comment #3 from Fabio Massimo Di Nitto <fdinitto at redhat.com>  2009-09-21 05:13:59 EDT ---
Hi Lon,

let's start with the basic stuff from the checklist:

rpmlint SPECS/* SRPMS/* RPM*/*/*
fence-virt-compat.x86_64: W: no-documentation
fence-virt-compat.x86_64: W: dangling-relative-symlink /usr/sbin/fence_xvm
fence_virt
fence-virtd.x86_64: W: no-documentation
fence-virtd-checkpoint.x86_64: W: no-documentation
fence-virtd-libvirt.x86_64: W: no-documentation
fence-virtd-multicast.x86_64: W: no-documentation
8 packages and 1 specfiles checked; 0 errors, 6 warnings.

The dangling-relative-symlink is OK, we will need to coordinate fence_xvm
deprecation from fence-agents package, but be aware that while we go through
the process, we will need a Conflicts (even if undesired), and later
fence-agents will require fence-virt-compat (a two stage commit).

For the documentation, read my comments below about the spec file.

MUST:

* The package must be named according to the  Package Naming Guidelines: OK
* The spec file name must match the base package: OK
* The package must meet the  Packaging Guidelines: see below details.
* The package must be licensed with a Fedora approved license: OK (GPLv2+)
* The License field in the package spec file must match the actual licens: OK
* 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 must be included in %doc: this needs to be done for each subpackage
(and it will also kill all the no-documentation warnings).
* The spec file must be written in American English: OK
* The spec file for the package MUST be legible: OK
* The sources used to build the package must match the upstream source: OK
* The package MUST successfully compile and build into binary rpms on at least
one primary architecture: OK
* All build dependencies must be listed in BuildRequires: NOT OK. The package
is missing some BuildRequires (nss and xml2 at least)
* The spec file MUST handle locales properly: does not apply
* Every binary RPM package (or subpackage) which stores shared library files:
does not apply
* Packages must NOT bundle copies of system libraries: OK
* A package must own all directories that it creates: NOT OK.
%{_libdir}/%{name}/ is not owned by the package or subpackages
* A Fedora package must not list a file more than once in the spec file's
%files listings: OK
* Permissions on files must be set properly: OK
* Each package must have a %clean section: OK
* Each package must consistently use macros: OK
* The package must contain code, or permissable conten: OK
* Large documentation files must go in a -doc subpackage: does not apply
* If a package includes something as %doc, it must not affect the runtime of
the application: OK
* Header files must be in a -devel package: does not apply.
* Static libraries must be in a -static package: does not apply.
* Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig': does not
apply.
* If a package contains library files with a suffix...: does not apply.
* In the vast majority of cases, devel packages must...: does not apply.
* Packages must NOT contain any .la libtool archives: OK
* Packages containing GUI applications must include a %{name}.desktop file:
does not apply
* Packages must not own files or directories already owned by other packages:
OK
* At the beginning of %install, each package MUST run rm -rf: OK
* All filenames in rpm packages must be valid UTF-8: OK

SHOULD:
* If the source package does not include license text: OK. upstream ships
licence file.
* The description and summary sections in the package spec file should contain
translations: does not apply
* The reviewer should test that the package builds in mock: NOK (missing
BuildRequires)
* The package should compile and build into binary rpms on all supported
architectures: (same as above)
* The reviewer should test that the package functions as described: OK
* If scriptlets are used, those scriptlets must be san: does not apply
* Usually, subpackages other than devel should require the base package: 
At a first glance it looks ok with the exception of:
fence-virtd-multicast Requires:       %{name}-host = but there is no -host
package?
* The placement of pkgconfig(.pc) files depends on their usecase..: does not
apply
* If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin,
..: OK.

A few notes on the spec file:

* URL should be in the form of:
http://fence-virt.git.sourceforge.net/fence-virt/fence-virt/fence-virt-%{version}.tar.gz
to include the file name to download.

* the format %{buildroot} is preferred over $RPM_BUILD_ROOT

* %install section can be simpler.
The whole installation of docs is redundant.

in the %files section use:
%doc COPYING TODO README

and you will achieve the same result. The macro %doc looks for files in the
unpacked source tree and copy them to the correct
%{_docdir}/%{name}-%{version}/
for you.

this will also allow you to copy COPYING and README into subpackages easily.

* the %configure macro is best expressed as %{configure} for consistency. Both
works.

Please fix the few remaining issues.

Fabio

-- 
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.