Bug 524105 - Review Request: fence-virt - Modular virtual machine fencing daemon
Summary: Review Request: fence-virt - Modular virtual machine fencing daemon
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Massimo Di Nitto
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-09-17 21:13 UTC by Lon Hohberger
Modified: 2009-12-07 16:36 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-09-25 18:17:09 UTC
fdinitto: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Corrected spec file (3.17 KB, text/plain)
2009-09-17 21:15 UTC, Lon Hohberger
no flags Details
fence-virt-0.1.2-1 spec file (3.04 KB, text/plain)
2009-09-23 19:07 UTC, Lon Hohberger
no flags Details

Description Lon Hohberger 2009-09-17 21:13:28 UTC
Spec URL: https://sourceforge.net/projects/fence-virt/files/fence-virt.spec/download

SRPM URL: https://sourceforge.net/projects/fence-virt/files/fence-virt-0.1-1.fc11.src.rpm/download

Description: A pluggable fencing framework for virtual machines

The goal is to eventually replace fence_xvm/fence_xvmd in fence-agents with the included functionality.  The reason this is a separate package from fence-agents is the fact that this agent would pull in a very wide range of dependencies which should be avoided.  For example, when a thin-hypervisor (e.g. oVirt) backend is written, the only requirements on the ring-0 operating system would be:

  fence-virtd-0.2
  fence-virtd-ovirt-0.2 (or whatever the plugin name is)

This division of dependencies will keep the ring-0 operating system image significantly smaller than would inclusion in the fence-agents package.  Furthermore, since the host package does not strictly depend on anything cluster related (indeed, it could be configured *without* cluster parts!), it is fairly logical to separate it.

[lhh@localhost rpm]$ rpmlint SPECS/fence-virt.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[lhh@localhost rpm]$ rpmlint SRPMS/fence-virt-0.1-1.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[lhh@localhost rpm]$ rpmlint RPMS/x86_64/fence-virt*
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
7 packages and 0 specfiles checked; 0 errors, 6 warnings.

Warnings:

Fence-virt-compat: The entire package is a symlink to fence_virt.  It correctly conflicts with fence-agents and requires the fence-virt package (to which it links).  Effectively, this symlink makes fence_virt behave like fence_xvm does today, and is wire-compatible with previous versions of fence_xvm.

Documentation: Several warnings are plugin packages which admittedly need documentation (for example how to use them!).  I hope to resolve these in a future release (say version 0.2).

Comment 1 Lon Hohberger 2009-09-17 21:15:04 UTC
Created attachment 361571 [details]
Corrected spec file

Evidently, the sourceforge.net propagated an incorrect copy of fence-virt.spec to its mirrors and is taking awhile to fix it.  Here is the correct spec file.

Comment 2 Lon Hohberger 2009-09-18 13:45:37 UTC
Spec files on sf.net have been updated.

Comment 3 Fabio Massimo Di Nitto 2009-09-21 09:13:59 UTC
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

Comment 4 Lon Hohberger 2009-09-21 14:52:19 UTC
fence-virt-0.1-2 spec file, based on Fabio's comments:

http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt.spec

Updated Source RPM:

http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt-0.1-2.fc11.src.rpm

Comment 5 Lon Hohberger 2009-09-23 14:47:19 UTC
I missed two things; I am addressing them now.

Comment 6 Lon Hohberger 2009-09-23 16:17:17 UTC
[root@localhost result]# rpmlint *rpm
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
7 packages and 0 specfiles checked; 0 errors, 4 warnings.

New specfile:

http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt.spec

New SRPM:

http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt-0.1.1-1.fc12.src.rpm

I have not built this in koji/scratch yet.

Comment 7 Lon Hohberger 2009-09-23 18:50:55 UTC

New Files:

http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt.spec
http://voxel.dl.sourceforge.net/project/fence-virt/fence-virt-0.1.2-1.fc12.src.rpm

[root@localhost ~]# rpmlint ~lhh/fence-virt.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

The source RPM is from the results of a mock run:

[root@localhost SPECS]# mock -r fedora-rawhide-x86_64  --rebuild /sandbox/lhh/rpm/SRPMS/fence-virt-0.1.2-1.fc11.src.rpm 
INFO: mock.py version 0.9.17 starting...
State Changed: init plugins
State Changed: start
INFO: Start(/sandbox/lhh/rpm/SRPMS/fence-virt-0.1.2-1.fc11.src.rpm)  Config(fedora-rawhide-x86_64)
State Changed: lock buildroot
State Changed: clean
State Changed: init
State Changed: lock buildroot
Mock Version: 0.9.17
INFO: Mock Version: 0.9.17
INFO: enabled root cache
State Changed: unpacking root cache
INFO: enabled yum cache
State Changed: cleaning yum metadata
INFO: enabled ccache
State Changed: running yum
State Changed: setup
State Changed: build
INFO: Done(/sandbox/lhh/rpm/SRPMS/fence-virt-0.1.2-1.fc11.src.rpm) Config(fedora-rawhide-x86_64) 1 minutes 57 seconds


Rpmlint on the mock results:

[root@localhost result]# rpmlint *rpm
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
7 packages and 0 specfiles checked; 0 errors, 4 warnings.

Ran this one through Koji --scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1701484

Comment 8 Lon Hohberger 2009-09-23 18:51:38 UTC
(There's still a delay for the fence-virt spec file update on sourceforge)

Comment 9 Lon Hohberger 2009-09-23 18:56:23 UTC
Oops -- sorry; wrong package spin.  Here's the correct koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1701613

Comment 10 Lon Hohberger 2009-09-23 19:07:38 UTC
Created attachment 362340 [details]
fence-virt-0.1.2-1 spec file

Comment 11 Fabio Massimo Di Nitto 2009-09-24 04:24:45 UTC
Verified that all the small glitches reported above have been fixed.

The package is good to go in.

Fabio

Comment 12 Lon Hohberger 2009-09-24 15:45:19 UTC
New Package CVS Request
=======================
Package Name: fence-virt
Short Description: Pluggable Virtual Machine Fencing
Owners: lon
Branches: F-12
InitialCC: fabbione

Comment 13 Lon Hohberger 2009-09-24 16:08:06 UTC
Strange; I've had fedora CVS package commit access for some time, yet I can't set fedora-cvs?

...

Comment 14 Fabio Massimo Di Nitto 2009-09-24 17:01:16 UTC
setting fedora-cvs? for lon

Comment 17 Kevin Fenzi 2009-09-25 16:15:39 UTC
cvs done.

Comment 18 Lon Hohberger 2009-12-07 14:44:45 UTC
Package Change Request
======================
Package Name: fence-virt
New Branches: F-11
Owners: lon fabbione

Comment 19 Kevin Fenzi 2009-12-07 16:36:22 UTC
cvs done.


Note You need to log in before you can comment on or make changes to this bug.