Bug 681558 - Review Request: libvirt-snmp - SNMP support for libvirt
Summary: Review Request: libvirt-snmp - SNMP support for libvirt
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tomas Mraz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-02 15:25 UTC by Michal Privoznik
Modified: 2013-10-19 14:42 UTC (History)
4 users (show)

Fixed In Version: libvirt-snmp-0.0.2-1.fc16
Clone Of:
Environment:
Last Closed: 2011-08-16 09:03:36 UTC
Type: ---
Embargoed:
tmraz: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Michal Privoznik 2011-03-02 15:25:07 UTC
Spec URL:
http://www.libvirt.org/git/?p=libvirt-snmp.git;a=blob;f=libvirt-snmp.spec.in;hb=HEAD

SRPM URL:
ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-1.fc14.src.rpm

Description:
This package adds SNMP support to libvirt. Currently it allows to get/set
domain status and view its actual resource usage. This package allows users
running many virtual machines to create end-to-end views, reports, control
vitrual machines lifecycles via SNMP, etc.

I am seeking for a sponsor.

$ rpmlint libvirt-snmp.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

$ rpmlint libvirt-snmp-0.0.1-1.fc15.i686.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint libvirt-snmp-0.0.1-1.fc15.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Koji scratch build:
F15: http://koji.fedoraproject.org/koji/taskinfo?taskID=2878572

Comment 1 Stanislav Ochotnicky 2011-03-02 16:25:45 UTC
Not a sponsor, just a quick look and few hints:
------
> Release:        1%{?dist}%{?extra_release}
should be just 
> Release:        1%{?dist}
------

%clean sections and rm -rf $RPM_BUILD_ROOT at the beginning of %install is no longer needed. (unless you plan to get this to EL-5)
------

Shouldn't this be %doc as well?
%{_datadir}/snmp/mibs/LIBVIRT-MIB.txt

------
I am guessing this will be automatically added by rpm when building package so no need for it:
Requires:       net-snmp

Comment 2 Michal Privoznik 2011-03-03 12:27:15 UTC
(In reply to comment #1)
> Not a sponsor, just a quick look and few hints:
> ------
> > Release:        1%{?dist}%{?extra_release}
> should be just 
> > Release:        1%{?dist}
> ------
> 
I disagree. It is there for performing our automated builds and for people who want to rebuild RPM and append unique string to identify their build. All our virt related RPMS have this.

Other hints taken in. URLs stays the same.

Comment 3 Tomas Mraz 2011-03-09 11:14:24 UTC
The libvirt tarball from the .src.rpm is different from the one at the URL. Please correct that.

If you update the .src.rpm and spec, please always bump the Release and add Changelog entry.

I noted that the license is GPLv3 - only - is that intentional? The source files do not contain any copyright comment - it is not required however it is recommended that they contain it (at least the non-autogenerated ones). There is the COPYING file that contains GPLv3 (which by itself allows the code to be licensed under the later future GPL licenses). However README states that the license is GPLv3 which limits it.

Why is the LIBVIRT-MIB.txt both in %doc and in the snmp/mibs data dir? I do not think it should be %doc - it is not in the net-snmp-libs package which contains most of the mib files.

Comment 4 Michal Privoznik 2011-03-10 14:30:53 UTC
Updated.

Spec URL:
http://www.libvirt.org/git/?p=libvirt-snmp.git;a=blob;f=libvirt-snmp.spec.in;hb=HEAD

SRPM URL:
ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-2.fc14.src.rpm

new tarball:
ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-2.tar.gz

LIBVIRT-MIB.txt is also in %doc, because it is part of documentation to allow users see what information they can gather/set.

Comment 5 Tomas Mraz 2011-03-10 15:19:33 UTC
(In reply to comment #4)
> Updated.
> 
> Spec URL:
> http://www.libvirt.org/git/?p=libvirt-snmp.git;a=blob;f=libvirt-snmp.spec.in;hb=HEAD
> 
> SRPM URL:
> ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-2.fc14.src.rpm
> 
> new tarball:
> ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-2.tar.gz

This does not make much sense - the tarball should stay the same if you're just changing the .spec file and not the source files. I know that you have the .spec.in in the upstream git repository however the Fedora spec should track the upstream spec only on rebases of the sources. So I recommend to use the 0.0.x versioning for the upstream tarball and have the spec file changes tracked only in the upstream git. Not to change the tarball with all the spec changes.

> LIBVIRT-MIB.txt is also in %doc, because it is part of documentation to allow
> users see what information they can gather/set.

This does not make much sense to me. The file is in the filesystem at one place and there is no reason to duplicate it as %doc somewhere else. Perhaps at most duplicate it as a symlink in the doc.

Comment 6 Stanislav Ochotnicky 2011-03-10 16:08:14 UTC
(In reply to comment #5)
> > LIBVIRT-MIB.txt is also in %doc, because it is part of documentation to allow
> > users see what information they can gather/set.
> 
> This does not make much sense to me. The file is in the filesystem at one place
> and there is no reason to duplicate it as %doc somewhere else. Perhaps at most
> duplicate it as a symlink in the doc.

That was my suggestion because looking at contents of rpm I noticed *.txt file that was not in %doc. I almost changed my mind, but I really believe this file can be useful for users/developers so I believe you should keep it in spec. Symlink will do of course. Would anyone care to explain why do these files even have *.txt extension? It's doesn't make much sense to me...

Comment 7 Michal Privoznik 2011-03-11 10:27:59 UTC
Updated.

SRPM URL:
ftp://libvirt.org/libvirt/snmp/libvirt-snmp-0.0.1-3.fc14.src.rpm

Comment 8 Tomas Mraz 2011-03-15 21:17:51 UTC
The rpmlint output:
rpmlint -v libvirt-snmp-0.0.1-3.fc13.src.rpm libvirt-snmp-0.0.1-3.fc13.x86_64.rpm libvirt-snmp-debuginfo-0.0.1-3.fc13.x86_64.rpm
libvirt-snmp.src: I: checking
libvirt-snmp.src: I: checking-url http://libvirt.org (timeout 10 seconds)
libvirt-snmp.src:40: W: macro-in-%changelog %doc
This is harmless, but please correct it before you import. Just remove the % so it is not interpreted.

libvirt-snmp.src: I: checking-url http://www.libvirt.org/sources/snmp/libvirt-snmp-0.0.1.tar.gz (timeout 10 seconds)
libvirt-snmp.x86_64: I: checking
libvirt-snmp.x86_64: I: checking-url http://libvirt.org (timeout 10 seconds)
libvirt-snmp-debuginfo.x86_64: I: checking
libvirt-snmp-debuginfo.x86_64: I: checking-url http://libvirt.org (timeout 10 seconds)
3 packages and 0 specfiles checked; 0 errors, 1 warnings.

Other than the non-blocker issue the rpmlint is silent.

The package seems to comply with Fedora packaging guidelines.

APPROVED.

I will sponsor you for fedora packager group.

Comment 9 Michal Privoznik 2011-03-16 10:47:18 UTC
New Package SCM Request
=======================
Package Name: libvirt-snmp
Short Description: Provide full snmp functionality for libvirtd
Owners: mprivozn
Branches: f15
InitialCC:

Comment 10 Jason Tibbitts 2011-03-16 12:25:39 UTC
Git done (by process-git-requests).


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