This, I think, is going to be a difficult one.

First, this takes a while to build, even on my 8-way box.  Is there some reason
why none of the make calls use %{?smp_mflags} ?

Now, loads upon loads of rpmlint warnings:

rpmlint of SRPM:
W: net-snmp strange-permission net-snmptrapd.init 0755
W: net-snmp strange-permission ucd5820stat 0755
W: net-snmp strange-permission net-snmpd.init 0755
W: net-snmp strange-permission net-snmp-config 0755
   I wouldn't worry about these; rpmlint just doesn't like executable files in
   an srpm.

W: net-snmp unversioned-explicit-obsoletes ucd-snmp
W: net-snmp unversioned-explicit-obsoletes ucd-snmp-utils
W: net-snmp unversioned-explicit-obsoletes ucd-snmp-devel
   I can't see any Fedora release where ucd-snmp was shipped.  These should
   just go away.

W: net-snmp rpm-buildroot-usage %build perl Makefile.PL -NET-SNMP-IN-SOURCE=true
PREFIX=${RPM_BUILD_ROOT}/%{_prefix} INSTALLDIRS=vendor -NET-SNMP-CONFIG="sh
../../net-snmp-config"
   rpmlint doesn't like to see the buildroot mentioned explicitly outside of
   %install, but in this case I think it's warranted.

W: net-snmp mixed-use-of-spaces-and-tabs (spaces: line 69, tab: line 161)
   It's good to be consistent with your indentation if possible, but this
   complaint is a bit odd in any case.

rpmlint of RPMs:
W: net-snmp incoherent-version-in-changelog 5.4-8 1:5.4-8.fc7
   I guess rpmlint wants to see the epoch in the changelog, but this isn't
   required by the guidelines.

E: net-snmp obsolete-not-provided ucd-snmp
E: net-snmp-devel obsolete-not-provided ucd-snmp-devel
E: net-snmp-utils obsolete-not-provided ucd-snmp-utils
   It's bad to obsolete a package without providing it.  But this should go
   away anyway.
 
E: net-snmp binary-or-shlib-defines-rpath /usr/sbin/snmpd
['/usr/lib64/perl5/5.8.8/x86_64-linux-thread-multi/CORE', '/usr/lib64']
E: net-snmp binary-or-shlib-defines-rpath /usr/sbin/snmptrapd
['/usr/lib64/perl5/5.8.8/x86_64-linux-thread-multi/CORE', '/usr/lib64']
E: net-snmp-perl binary-or-shlib-defines-rpath
/usr/lib64/perl5/vendor_perl/5.8.8/x86_64-linux-thread-multi/auto/NetSNMP/TrapReceiver/TrapReceiver.so
['/usr/lib64', '/usr/lib64/perl5/5.8.8/x86_64-linux-thread-multi/CORE']
   I've never seen the Perl module path in one of these warnings, and it
   seems really odd to me.  Even if rpath was permissible, why would a
   compiled executable need an rpath including a Perl module directory?

E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpset ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpusm ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/encode_keychange
['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpdelta ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmptest ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpstatus ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpdf ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpwalk ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmptrap ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpbulkget ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpnetstat ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpget ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmptable ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpvacm ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmptranslate
['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpgetnext ['/usr/lib64']
E: net-snmp-utils binary-or-shlib-defines-rpath /usr/bin/snmpbulkwalk ['/usr/lib64']
   These are at least the regular form of this error, but still need to be
   fixed.  The extras buildsys wouldn't build this package due to these.

E: net-snmp script-without-shebang /usr/share/snmp/mib2c.perl.conf
E: net-snmp script-without-shebang /usr/share/snmp/snmp_perl.pl
E: net-snmp script-without-shebang /usr/share/snmp/snmp_perl_trapd.pl
E: net-snmp script-without-shebang /usr/share/snmp/mib2c.row.conf
E: net-snmp-perl script-without-shebang
/usr/lib64/perl5/vendor_perl/5.8.8/x86_64-linux-thread-multi/NetSNMP/agent/Support.pm
   Why are these executable?  If they're meant to be executed, they need to
   have shebang lines.

E: net-snmp incoherent-logrotate-file /etc/logrotate.d/snmpd
   rpmlint wants to see the logrotate file named after the package, but I
   think this is bogus in this case.

W: net-snmp spurious-executable-perm /usr/share/doc/net-snmp-5.4/ipf-mod.pl
W: net-snmp spurious-executable-perm /usr/share/doc/net-snmp-5.4/passtest
W: net-snmp doc-file-dependency /usr/share/doc/net-snmp-5.4/ipf-mod.pl /usr/bin/perl
   Documentation should not be executable.

E: net-snmp executable-marked-as-config-file /etc/rc.d/init.d/snmptrapd
E: net-snmp executable-marked-as-config-file /etc/rc.d/init.d/snmpd
   Files in init.d don't generally need to be marked %config and certainly
   shouldn't be marked noreplace.
 
W: net-snmp dangerous-command-in-%preun rm
   Shouldn't the file that's deleted be %ghost'ed instead?

W: net-snmp-debuginfo spurious-executable-perm
/usr/src/debug/net-snmp-5.4/agent/helpers/table_row.c
   The source should definitely not be executable.

W: net-snmp-devel summary-ended-with-dot The development environment for the
NET-SNMP project.
W: net-snmp-libs summary-ended-with-dot The NET-SNMP runtime libraries.
W: net-snmp-perl summary-ended-with-dot The perl NET-SNMP module and the mib2c tool.
W: net-snmp-utils summary-ended-with-dot Network management utilities using
SNMP, from the NET-SNMP project.
   Trivial to fix.

W: net-snmp-libs no-documentation
   This is OK.

E: net-snmp-perl non-standard-executable-perm
/usr/lib64/perl5/vendor_perl/5.8.8/x86_64-linux-thread-multi/NetSNMP/agent/Support.pm
0555
   Should be mode 0755, unless there's some reason for it to be this way.
   
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpagent.so.15.0.0
netsnmp_udp_parse_security
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpagent.so.15.0.0
netsnmp_UnixDomain
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpagent.so.15.0.0
netsnmp_udp6_parse_security
W: net-snmp-libs undefined-non-weak-symbol
/usr/lib64/libnetsnmphelpers.so.15.0.0 snmp_free_var
W: net-snmp-libs undefined-non-weak-symbol
/usr/lib64/libnetsnmphelpers.so.15.0.0 netsnmp_ncompare_netsnmp_index
W: net-snmp-libs undefined-non-weak-symbol
/usr/lib64/libnetsnmphelpers.so.15.0.0 netsnmp_compare_netsnmp_index
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmptrapd.so.15.0.0
netsnmp_snmpTCPDomain
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmptrapd.so.15.0.0
dropauth
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmptrapd.so.15.0.0
SyslogTrap
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpmibs.so.15.0.0
snmp_enableauthentrapsset
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpmibs.so.15.0.0
usmNoPrivProtocol
W: net-snmp-libs undefined-non-weak-symbol /usr/lib64/libnetsnmpmibs.so.15.0.0
usmNoAuthProtocol
   [etc...]
   There are several hundred of these.  Generally we try to fix them if we
   can, because they can cause linkage errors or code using this library.

W: net-snmp-libs unused-direct-shlib-dependency
/usr/lib64/libnetsnmpagent.so.15.0.0 /lib64/libcrypto.so.6
W: net-snmp-libs unused-direct-shlib-dependency
/usr/lib64/libnetsnmphelpers.so.15.0.0 /lib64/libcrypto.so.6
W: net-snmp-libs unused-direct-shlib-dependency
/usr/lib64/libnetsnmptrapd.so.15.0.0 /lib64/libcrypto.so.6
W: net-snmp-libs unused-direct-shlib-dependency
/usr/lib64/libnetsnmpmibs.so.15.0.0 /lib64/libcrypto.so.6

And a few other things found in review:

I can't fetch the source from the Source0: URL; there is no FTP site
at net-snmp.sourceforge.net.  The Source0: URL should, I think, be 
http://dl.sourceforge.net/net-snmp/net-snmp-%{major_ver}.tar.gz

BuildRoot should be 
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

I think it's best to just use a license tag of "BSD", since even the
"BSD-like" CMU license is really just BSD.

The scriptlet dependencies are a bit odd; there is only
   Requires(pre): /sbin/chkconfig
but there's no %pre scriptlet.  You should use:
   Requires(post): /sbin/chkconfig
   Requires(preun): /sbin/chkconfig
   Requires(preun): /sbin/service
   Requires(preun): /bin/rm
and if you're going to call chkconfig with a full path, you should do the same
for service.

There's a test suite; has anyone looked into running it at build time?

The -devel package includes static libs, which should not generally be shipped
in Fedora.

Review:
* source files match upstream:
   2f43cd6f3c4066f8c17fdc47931a96c1fce808c9d1dd74bcb5a79d9d29d5f947
   net-snmp-5.4.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
   (Well, the spec is honestly a bit messy, but it's a messy package.)
* dist tag is present.
X build root is not correct.
? license field matches the actual license.
   Probably best to just say "BSD" here.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper
   (You don't actually need to list perl, coreutils, grep, sed, or findutils.)
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has many valid complaints.
* final provides and requires seem sane:
  net-snmp-5.4-8.fc7.x86_64.rpm
   config(net-snmp) = 1:5.4-8.fc7
   net-snmp = 1:5.4-8.fc7
  =
   /bin/bash
   /bin/sh
   /sbin/chkconfig
   /usr/bin/perl
   config(net-snmp) = 1:5.4-8.fc7
   libcrypt.so.1()(64bit)
   libcrypto.so.6()(64bit)
   libnetsnmp.so.15()(64bit)
   libnetsnmpagent.so.15()(64bit)
   libnetsnmphelpers.so.15()(64bit)
   libnetsnmpmibs.so.15()(64bit)
   libnetsnmptrapd.so.15()(64bit)
   libperl.so()(64bit)
   libsensors.so.3()(64bit)
   libwrap.so.0()(64bit)

  net-snmp-devel-5.4-8.fc7.x86_64.rpm
   net-snmp-devel = 1:5.4-8.fc7
  =
   /bin/sh
   beecrypt-devel
   elfutils-devel
   elfutils-libelf-devel
   libnetsnmp.so.15()(64bit)
   libnetsnmpagent.so.15()(64bit)
   libnetsnmphelpers.so.15()(64bit)
   libnetsnmpmibs.so.15()(64bit)
   libnetsnmptrapd.so.15()(64bit)
   libsnmp.so.15()(64bit)
   lm_sensors-devel
   net-snmp = 1:5.4
   rpm-devel
   tcp_wrappers-devel

  net-snmp-libs-5.4-8.fc7.x86_64.rpm
   libnetsnmp.so.15()(64bit)
   libnetsnmpagent.so.15()(64bit)
   libnetsnmphelpers.so.15()(64bit)
   libnetsnmpmibs.so.15()(64bit)
   libnetsnmptrapd.so.15()(64bit)
   libsnmp.so.15()(64bit)
   net-snmp-libs = 1:5.4-8.fc7
  =
   /sbin/ldconfig
   libcrypto.so.6()(64bit)
   libnetsnmp.so.15()(64bit)
   libnetsnmpagent.so.15()(64bit)
   libnetsnmphelpers.so.15()(64bit)
   libnetsnmpmibs.so.15()(64bit)
   libnetsnmptrapd.so.15()(64bit)
   libsnmp.so.15()(64bit)

? %check is not present but there seems to be a test suite.
* shared libraries present; ldconfig is called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
X file permissions are appropriate.
   (various executable source files and bits of documentation)
X scriptlets present without proper dependencies.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel subpackage.
* no pkgconfig files.
* no libtool .la droppings.
X static libraries are present

Hi, thanks for the review. This will be a tricky one :) 
Here are two comments I have
- perl scripts in doc are examples, I guess having them executable makes sense 
- the test suit fails on s390* and I don't have a fix for that
- ucd-snmp is used in RHEL2.1 - but I'm willing to remove that cos ucd-snmp is
really ancient as well as 2.1

I have no idea how to deal with the rpath issue and also I have to check why the
library in devel is static. Patches welcome. 

The problem with executable documentation is that it often creates dependencies
which should not be there.  For example, the /usr/bin/perl dependency comes from
the documentation.  This package does depend on libperl.so so it turns out that
this doesn't happen to be dependency bloat, but if someone ever decided to split
off a perl-libs package then you'd be needlessly pulling in the Perl
interpreter.  Still, there's no guideline in place against

You could always skip the test suite on s390 using %ifarch, if you think there's
real benefit to running it.

Upgrades certainly aren't supported from RHEL2.1 to RHEL5 (or obviously any
Fedora release) so nobody could argue that as a reason to keep the Obsoletes around.

I do wish I could help with patches, but unfortunately I know little of the
internals of this piece of software and there are something like 900 more core
packages that need reviewing.  If I have the time I will try to assist in at
least fixing the specfile issues.

By the soon to be ratified process, further work is required by the package
owner, (and even though it isn't entirely logical) NEEDINFO is to point at the
package owner until that work is done.

Just a note that I've talked it over with the packaging committee and such and
the concensus is that there's no problem with executable documentation as long
as it doesn't cause additional dependencies, which it doesn't in this case. So
we're good there.

Huh, the review is still open... I should have noticed it earlier. It has been quite long time since the review has started and now the net-snmp package looks differently and hopefuly simpler.

I have just fixed the errors noted above and prepared net-snmp-5.5-3.fc13 in rawhide, which should be review-able again.

X build root is not correct.
Fixed

? license field matches the actual license.
Fixed (BSD and MIT)

? %check is not present but there seems to be a test suite.
I've added the %check. I am not sure it will work when the package is compiled on system with net-snmp already installed. It works in mock and in koji though. The whole %check is not executed when %{netsnmp_check} == 0 to speed up my local builds.

X file permissions are appropriate.
rpmlint still complains about executable flag on agent/helpers/table_row.c,
I have fixed it upstream, see http://net-snmp.svn.sourceforge.net/viewvc/net-snmp?view=rev&revision=17839 . I hope it's enough - I don't want to add another .spec voodoo just because of this one file.

X scriptlets present without proper dependencies.
Fixed

X rpmlint has many valid complaints.
Fixed most of them, following remain:
net-snmp.i686: W: dangerous-command-in-%post mv
net-snmp.x86_64: W: dangerous-command-in-%post mv
There is one mv, which moves net-snmp data from /var/net-snmp to /var/lib/net-snmp, IMHO it's valid usage.

net-snmp.src: W: strange-permission net-snmp-config 0755
net-snmp.src: W: strange-permission net-snmpd.init 0755
net-snmp.src: W: strange-permission net-snmptrapd.init 0755
These files are stored in CVS with executable bit set, AFAIK there is no easy way how to remove it. And IMHO it's pretty harmless.

net-snmp.src: W: %ifarch-applied-patch Patch1: net-snmp-5.4.1-pie.patch
IA64 has some problems with -pie, I'll look into it. Maybe it's not needed anymore - is IA64 secondary architecture?

Indeed the package does look better.  Still quite a number of rpmlint complaints, most of which can be ignored, but I'll post the full list:

  net-snmp.x86_64: W: dangerous-command-in-%post mv
Agreed that this may be OK, but what was the last release that put data in /var/net-snmp?  If it was more than a few releases ago, there's no point in keeping this bit at all.  At the least, some indication of which releases require it is in order.

Similarly, "allow compilation on old Fedoras" can go if "old Fedoras" is older than F10.  At the very least, some statement of "how old" would be good to have.

  net-snmp-debuginfo.x86_64: W: spurious-executable-perm
   /usr/src/debug/net-snmp-5.5/agent/helpers/table_row.c
If you fixed it upstream, great; this isn't really that big of a deal anyway.  Honestly it would be nice if the debuginfo generation took care of that kind of thing anyway.

  net-snmp-libs.x86_64: W: undefined-non-weak-symbol
   /usr/lib64/libnetsnmpagent.so.20.0.0 netsnmp_register_null_context
Six of these in this library with different symbols.  I suppose if you want to use this library you also have to link against some other library which provides those symbols.  That's bad programming practice but not a review blocker.

  net-snmp-libs.x86_64: W: unused-direct-shlib-dependency
   /usr/lib64/libnetsnmpagent.so.20.0.0 /usr/lib64/libcrypto.so.10  
Many of these in various different libraries.  All of these libraries are linked against other libraries even though they call nothing in them.  This probably isn't an issue as most of these libraries will be in memory anyway, although there are a couple of libperl.so references that don't need to be there.
In general these are OK; they're bad if the force unnecessary dependencies or pull in unused libraries that won't generally already be in memory, and you can get rid of them with "-Wl,--as-needed" on the link line.  Maybe that's what "--enable-as-needed" on the configure line is for, but it doesn't seem to help.  In any case I don't see anything which would block a review.

One minor nit is that I don't see the point of the "Building option" bit in %description.  When you see that in the final package, it makes it seem that use of tcp_wrappers is disabled when in fact it's enabled.  Such a comment is appropriate for the spec file but not really useful in the binary package.

So really, this is very close.

(In reply to comment #7)
>   net-snmp.x86_64: W: dangerous-command-in-%post mv
> Agreed that this may be OK, but what was the last release that put data in
> /var/net-snmp?  If it was more than a few releases ago, there's no point in
> keeping this bit at all.  At the least, some indication of which releases
> require it is in order.

Fedora 11: /var/net-snmp, Fedora 12: /var/lib/net-snmp. I've added a note about it.


> Similarly, "allow compilation on old Fedoras" can go if "old Fedoras" is older
> than F10.  At the very least, some statement of "how old" would be good to
> have.

Again, rpm on Fedora 11 does not define %python_sitearch, I've added a comment there.


>   net-snmp-libs.x86_64: W: undefined-non-weak-symbol
>    /usr/lib64/libnetsnmpagent.so.20.0.0 netsnmp_register_null_context
> Six of these in this library with different symbols.  I suppose if you want to
> use this library you also have to link against some other library which
> provides those symbols.  That's bad programming practice but not a review
> blocker.

This one is broken upstream, libnetsnmpagent.so does not link with libsnmphelpers.so, but needs its symbols. In fact, libsnmphelpers and libnetsnmpagent need symbols from each other :). I've filled a bug upstream and I'll look at it eventually.
https://sourceforge.net/tracker/?func=detail&atid=112694&aid=2930536&group_id=12694


>   net-snmp-libs.x86_64: W: unused-direct-shlib-dependency
>    /usr/lib64/libnetsnmpagent.so.20.0.0 /usr/lib64/libcrypto.so.10  
> Many of these in various different libraries.  All of these libraries are
> linked against other libraries even though they call nothing in them.  This
> probably isn't an issue as most of these libraries will be in memory anyway,
> although there are a couple of libperl.so references that don't need to be
> there.
> In general these are OK; they're bad if the force unnecessary dependencies or
> pull in unused libraries that won't generally already be in memory, and you can
> get rid of them with "-Wl,--as-needed" on the link line.  Maybe that's what
> "--enable-as-needed" on the configure line is for, but it doesn't seem to help.

Strange, it looks like --enable-as-needed does absolutely nothing. I'll check it with upstream, this looks like a silly bug.


>  In any case I don't see anything which would block a review.
Thanks a lot for the review! Would you kindly approve it?


> One minor nit is that I don't see the point of the "Building option" bit in
> %description.  When you see that in the final package, it makes it seem that
> use of tcp_wrappers is disabled when in fact it's enabled.  Such a comment is
> appropriate for the spec file but not really useful in the binary package.

"Building option" removed.

net-snmp-5.5-10.fc13 with aforementioned changes is just being built

Mass reassigning all merge reviews to their component. For more details, see this FESCO ticket:

  https://fedorahosted.org/fesco/ticket/1269

If you don't know what merge reviews are about, please see:

  https://fedoraproject.org/wiki/Merge_Reviews

How to handle this bug is left to the discretion of the package maintainer.

This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

This message is a reminder that Fedora 23 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 23. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '23'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 23 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Fedora 23 changed to end-of-life (EOL) status on 2016-12-20. Fedora 23 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.

This bug appears to have been reported against 'rawhide' during the Fedora 26 development cycle.
Changing version to '26'.