Bug 858060 - Review Request: qpid-snmpd - SNMP agent for qpid broker
Summary: Review Request: qpid-snmpd - SNMP agent for qpid broker
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Tom "spot" Callaway
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW 875137 875138
TreeView+ depends on / blocked
 
Reported: 2012-09-17 22:24 UTC by Ernie
Modified: 2020-06-13 15:27 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-06-13 15:27:38 UTC
Type: Bug
Embargoed:
tcallawa: fedora-review+
eallen: needinfo-
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ernie 2012-09-17 22:24:45 UTC
Darrly Pierce (dpierce) has volunteered to review. Please allow him to take the review. 

SPEC url: http://eallen.fedorapeople.org/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-1.fc17.src.rpm
Description: SNMP agent based on net-snmp that provides read-only access to query the properties of a qpid broker.

Comment 1 Darryl L. Pierce 2012-09-18 14:31:36 UTC
Generally good, just a few issues to fix.


LEGEND: X=Met, !=Not met, ?=Not a blocker but should be fixed
=============================================================
[X]  MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1]

mcpierce@mcpierce-laptop:review  $ ll
total 836
-rw-rw-r--. 1 mcpierce mcpierce 184796 Sep 17 18:37 qpid-snmpd-1.0.0-1.fc17.src.rpm
-rw-rw-r--. 1 mcpierce mcpierce 148069 Sep 18 09:49 qpid-snmpd-1.0.0-1.fc17.x86_64.rpm
-rw-rw-r--. 1 mcpierce mcpierce 503137 Sep 18 09:49 qpid-snmpd-debuginfo-1.0.0-1.fc17.x86_64.rpm
-rw-rw-r--. 1 mcpierce mcpierce   2229 Sep 17 18:38 qpid-snmpd.spec
-rw-rw-r--. 1 mcpierce mcpierce   4845 Sep 18 09:42 review
mcpierce@mcpierce-laptop:review  $ rpmlint *rpm
qpid-snmpd.x86_64: W: manual-page-warning /usr/share/man/man8/qpid-snmpd.8.gz 21: warning: macro `BI(default).' not defined (possibly missing space after `BI')
qpid-snmpd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/qpid-snmpd $name
qpid-snmpd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/qpid-snmpd $name
qpid-snmpd.x86_64: W: incoherent-subsys /etc/rc.d/init.d/qpid-snmpd $name
qpid-snmpd.x86_64: W: service-default-enabled /etc/rc.d/init.d/qpid-snmpd
3 packages and 0 specfiles checked; 0 errors, 5 warnings.

[X]  MUST: The package must be named according to the Package Naming Guidelines .
[X]  MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] . 
[X]  MUST: The package must meet the Packaging Guidelines .
[X]  MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
[X]  MUST: The License field in the package spec file must match the actual license. [3]
[X]  MUST: 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.[4]
[X]  MUST: The spec file must be written in American English. [5]
[X]  MUST: The spec file for the package MUST be legible. [6]
[X]  MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
[X]  MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
[X]  MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
[X]  MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
[X]  MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]
[X]  MUST: Packages must NOT bundle copies of system libraries.[11]
[!]  MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]

The two %doc files are not in proper locations, but are instead listed as being in %{_build_dir} in the %files section.
They should instead use %{_docdir}/%{name}-%{version}

[X]  MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)[14]
[!]  MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. [15]

Two different methods are used to install files in the %install section: cp and install, with the former leaving permissions unverified.
Please use install consistently.

[!]  MUST: Each package must consistently use macros. [16]

1) See above note regarding the location of the license.txt and README.txt. 

2) The use of the %{_build_dir} is confusing since it could be misread as an alias for %{buildroot}. Instead, please use %{buildroot} and use "%setup -q -c %{name}-%{version}" to explode the sources.

[X]  MUST: The package must contain code, or permissable content. [17]
[X]  MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]
[X]  MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]
[X]  MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[19]
[X]  MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23]
[X]  MUST: All filenames in rpm packages must be valid UTF-8. [24]

Comment 2 Ernie 2012-09-18 19:12:06 UTC
SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd/1.0.0-2.fc17/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd/1.0.0-2.fc17/qpid-snmpd-1.0.0-2.fc17.src.rpm

Please note that the spec and srpm locations have changed.

Thanks Darryl.
>[!]  MUST: Permissions on files must be set properly. Executables should be >set with executable permissions, for example. [15]
>
>Two different methods are used to install files in the %install section: cp >and install, with the former leaving permissions unverified.
>Please use install consistently.

Done

>[!]  MUST: Each package must consistently use macros. [16]
>
>1) See above note regarding the location of the license.txt and README.txt. 
>
>2) The use of the %{_build_dir} is confusing since it could be misread as an >alias for %{buildroot}. Instead, please use %{buildroot} and use "%setup -q -c >%{name}-%{version}" to explode the sources.

I've changed the directory into which the source is exploded (in the zip file). Before it was the same name as the app (qpid-snmpd). That caused naming problems. I've changed the name of the global to avoid confusion with the %{builddir}

>[!]  MUST: A package must own all directories that it creates. If it does not >create a directory that it uses, then it should require a package which does >create that directory. [13]
>
>The two %doc files are not in proper locations, but are instead listed as >being in %{_build_dir} in the %files section.
>They should instead use %{_docdir}/%{name}-%{version}
 
Sorry, but I'm confused by this. When installed, the current rpm puts the README.txt and license.txt files in the /usr/share/doc/qpid-snmpd-1.0.0 directory. Is that not correct? The lines in the spec file use the %doc macro to do the install so they are put in the proper location. They just reference the %{buildroot} relative location of the files as the source location. Should I change the source location? 

Also, even though you accepted the rpmlint warnings, is there anything I can do to get rid of them?

Comment 3 Ernie 2012-09-20 15:24:51 UTC
SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-3.fc17.src.rpm
TAR  URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0.tar.bz2

Reverted to original tar.bz2 to avoid re-running review tests.

Spec file was updated to remove the %{source-dir} global. Now uses pushd and pop with the original tar.bz2 source directory name. For future packages, I'll just place the source in the root of the tar.bz2 to avoid these problems.

The "[!] not met" sections have been addressed.

Comment 4 Darryl L. Pierce 2012-09-20 17:25:18 UTC
(In reply to comment #3)
> SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd.spec
> SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-3.fc17.src.rpm
> TAR  URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0.tar.bz2
> 
> Reverted to original tar.bz2 to avoid re-running review tests.
> 
> Spec file was updated to remove the %{source-dir} global. Now uses pushd and
> pop with the original tar.bz2 source directory name. For future packages,
> I'll just place the source in the root of the tar.bz2 to avoid these
> problems.

No need to do that. You should keep the directory name within the tarball as %{name}-%{version}, then use

%setup -q -c %{name}-%{version}
pushd %{name}-%{version}

That way you can always know in your source tarball which version of the code it represents, should the filename somehow get munged.

> The "[!] not met" sections have been addressed.

The rpmlint warnings aren't show stoppers, so you can fix them after this review.

The main complaint (the "incoherent-subsys" message) is due to the /etc/rc.d/init.d/qpid-snmpd script using "$name" rather than explicitly stating the name of the service. Since the script's not re-used, just replace "$name" with "qpid-snmpd" and that error will go away.

The other complaint ("service-default-enabled") is because the installation enables the service by default. That's not really supposed to be done: the service should be added, but the administrator should be the one to set it to on  explicitly.

Those two things should be fixed before pushing out the first build.

The issue with the %doc files is fixed.
The issue with permissions on a few of the installed files is fixed.

All in all, this looks good. With the above caveats about the rpmlint errors, I'll go ahead and approve this package.

Comment 5 Ernie 2012-09-20 18:25:10 UTC
I've changed the /init.d/qpid-snmpd script to
- Explicitly use qpid-snmpd instead of $name.
- Not enable the service by default.

rpmlint is now silent.

Thanks for review Darryl.

Comment 6 Ernie 2012-09-21 12:35:00 UTC
New Package SCM Request
=======================
Package Name: qpid-snmpd
Short Description: SNMP agent for Amqp qpid broker
Owners: eallen mpierce
Branches: f16 f17 f18 el6
InitialCC: eallen

Comment 7 Gwyn Ciesla 2012-09-21 12:41:19 UTC
WARNING: "eallen" is not in the packager group.
WARNING: "mpierce" is not a valid FAS account.
WARNING: Couldn't parse package name out of bug summary.

Comment 8 Ernie 2012-09-26 13:56:23 UTC
Changed from an init.d script to a unit file.
Removed the step to zip the man page.

SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-4.fc17.src.rpm
TAR  URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0.tar.bz2

One thing of note:
Installing the qpid-snmpd.service file in %{_unitdir} does generate an rpmlint warning:

> [eallen@redhat x86_64]$ rpmlint *rpm ../../SPECS/qpid-snmpd.spec
> qpid-snmpd.x86_64: W: only-non-binary-in-usr-lib
> 2 packages and 1 specfiles checked; 0 errors, 1 warnings.

But it appears that this warning is bogus:
https://bugzilla.redhat.com/show_bug.cgi?id=794777

Comment 9 Tom "spot" Callaway 2012-10-02 18:17:16 UTC
Hi Ernie, a few points:

1) You're using the older systemd scriptlets here. That's fine for Fedora 17 or older, but just be sure to use the newer macroized scriptlets for Fedora 18+ targets (https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd). You can either conditionalize this in the spec, or you can make the change in the F18 and "master" branches in git.

2) You also do not need to define a BuildRoot, delete the BuildRoot in %install, or define a default %clean section, unless you're building for EPEL (which I happen to know from talking to Darryl that you are), so just be aware of that for any future Fedora only packages.

3) You have several config files in this package, I assume they are being used as opposed to being included solely for reference. If they are being used, please mark them as %config(noreplace) in %files:

%config(noreplace) %{_datadir}/snmp/qpid010.conf

This ensures that these config files are not replaced when this package is upgraded.

******

Clarify these items for me, and I'll go ahead and finish off this review. Sorry for the delay! :)

Comment 10 Ernie 2012-10-02 19:53:36 UTC
SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-5.fc17.src.rpm

> 1) ... use the newer macroized scriptlets for Fedora 18+...
I added the conditionals to the spec file for Fedora 18+ for the systemd scriptlets.

> 2) ... do not define a BuildRoot, delete the BuildRoot... unless you're 
> building for EPEL.
Thanks. For future Fedora only packages, I'll be sure to avoid those steps.

> 3) ... If they (the config files) and being used, please mark them as 
> %config(noreplace) in %files
I marked the config files noreplace in the %files section of the spec.
However, doing so has added some rpmlint warnings:
>qpid-snmpd.x86_64: W: non-etc-or-var-file-marked-as-conffile 
>/usr/share/snmp/qpid_snmp.conf
>qpid-snmpd.x86_64: W: non-etc-or-var-file-marked-as-conffile 
>/usr/share/snmp/qpid010.conf
But the files can't be moved. The net-snmp libraries used to build the application require that the config files be in the /usr/share/snmp directory.

Thanks Tom!

Comment 11 Tom "spot" Callaway 2012-10-03 13:16:11 UTC
One thing I missed before, you don't have proper scriptlet requires, specifically:

Requires(post): systemd-units
Requires(preun): systemd-units
Requires(postun): systemd-units

Add those and we'll be all done. Oh, and please tell me what your FAS account username is so I can sponsor you.

Comment 12 Ernie 2012-10-03 16:46:15 UTC
SPEC URL: http://eallen.fedorapeople.org/qpid-snmpd.spec
SRPM URL: http://eallen.fedorapeople.org/qpid-snmpd-1.0.0-6.fc17.src.rpm

Added the Requires(post,preun,postun) statements.

My FAS account is eallen

Thanks Tom and Darryl.

Comment 13 Tom "spot" Callaway 2012-10-03 17:04:24 UTC
APPROVED.

Comment 14 Ernie 2012-10-04 15:09:32 UTC
New Package SCM Request
=======================
Package Name: qpid-snmpd
Short Description: SNMP agent for Amqp qpid broker
Owners: eallen
Branches: f16 f17 f18 el6
InitialCC: mcpierce

Comment 15 Gwyn Ciesla 2012-10-04 15:31:53 UTC
Git done (by process-git-requests).

Comment 16 Mattia Verga 2020-05-30 13:56:01 UTC
This package was approved long time ago and the git repo was set up, but the package itself has never been imported.
If you don't want to maintain it, please retire the package from the git repo and/or orphan it.

Comment 17 Ernie 2020-06-03 19:08:00 UTC
Irina, If this was never imported, can you retire the package or orphan it. I'm not sure how to do either.

Comment 18 Ernie 2020-06-11 18:56:31 UTC
I will not be maintaining this package. I recommend the package be retired since it was never imported. I do not know how to retire or orphan this package.

Comment 19 Mattia Verga 2020-06-13 15:27:38 UTC
Package has been retired from Fedora Rawhide.
Closing this ticket as dead review.


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