Bug 532402

Hi Mark,

I'm not a sponsor, I just have some general recommendations about your package.  This is my first review, so please let me know if there are any questions.

1. The package didn't build on my system due to permissions on directories on /etc/apf

http://boodle.fedorapeople.org/RPMS/error.out

2. rpmlint has a few messages:
$ rpmlint apf.spec
apf.spec: W: no-cleaning-of-buildroot %install
apf.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 1)

$ rpmlint ../SRPMS/apf-9.7-1.src.rpm
apf.src: W: no-version-in-last-changelog
apf.src: W: no-cleaning-of-buildroot %install
apf.src: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 1)
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

Please correct these.

3. Your buildroot is probably fine, but Fedora does have preferences, please use a buildroot from:
https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag


4. source0 should be URL to source file, please see:
https://fedoraproject.org/wiki/Packaging:SourceURL

5. Add a version to the last changelog entry

(In reply to comment #1)

Thanks for taking the time to look at my first package.

> 1. The package didn't build on my system due to permissions on directories on
> /etc/apf

Fixed.

> 2. rpmlint has a few messages:

Fixed.

> 3. Your buildroot is probably fine, but Fedora does have preferences, please
> use a buildroot from:
> https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

Fixed.

> 4. source0 should be URL to source file, please see:
> https://fedoraproject.org/wiki/Packaging:SourceURL

Unfortunately the author does not provide a way to download a specific version, you can only download the latest -current. I've added a note to the spec file explaining the situation.

> 5. Add a version to the last changelog entry  

Done.

The updated SPEC and SRPM are at:

http://mmckinst.nexcess.net/apf/apf.spec
http://mmckinst.nexcess.net/apf/apf-9.7.1-2.src.rpm

No problem.  Just a couple more things here.  Please make the changes and I'll have another look.

1. Now it does build.  rpmlint has issues with the rpm:

$ rpmlint ../RPMS/noarch/apf-9.7.1-2.noarch.rpm
apf.noarch: E: non-readable /etc/apf/allow_hosts.rules 0640
apf.noarch: E: non-readable /etc/apf/internals/multicast.networks 0640
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/multicast.networks
apf.noarch: E: non-readable /etc/apf/apf 0750
apf.noarch: E: non-standard-executable-perm /etc/apf/apf 0750
apf.noarch: E: non-readable /etc/apf/internals/private.networks 0640
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/private.networks
apf.noarch: E: non-readable /etc/apf/VERSION 0640
apf.noarch: W: non-conffile-in-etc /etc/apf/VERSION
<snip>
1 packages and 0 specfiles checked; 55 errors, 21 warnings.

You can get information on these errors / warnings here:

https://fedoraproject.org/wiki/Common_Rpmlint_issues

You can also get more verbose info with "rpmlint -i"

2. I'd take out the 

BuildArch:      noarch

tag.

3. add the %{?dist} tag to release.

4. Please don't chkconfig a service on by default:
chkconfig --level 345 apf on

See 

https://fedoraproject.org/wiki/Packaging:SysVInitScript#Why_don.27t_we....

5. instead of defining basedir:

%define         basedir /etc/apf

you can use the %{_sysconfdir} tag.

which would change:

find %{buildroot}%{basedir}/ -type f -exec chmod 640 {} \;
to:
find %{buildroot}%{_sysconfdir}/apf -type f -exec chmod 640 {} \;

and a few other places as well...

6. You can take out these two lines:

mkdir -p %{buildroot}/%{_docdir}/%{name}
cp -pf COPYING.GPL CHANGELOG README.apf %{buildroot}/%{_docdir}/%{name}/

and use the %doc tag (something like):
%doc COPYING.GPL CHANGELOG README.apf

so, in your %files, 
%doc %{_docdir}/apf/CHANGELOG

take out the %{_docdir}/apf/

ping

I'm still alive, I've just been busy. I'm planning on working on the APF rpm this weekend.

> 1. Now it does build.  rpmlint has issues with the rpm:

APF is kind of weird in the way its written. It is essentially a collection of shell scripts that act as a wrapper to iptables. It, by default, installs everything including executables in /etc/apf . The documentation advises against changing the install directory so I have went with the authors advice. I went through what rpmlint said and commented on it.

> apf.noarch: E: non-executable-script /etc/apf/conf.apf 0640 /bin/bash
> apf.noarch: E: non-executable-script /etc/apf/extras/dshield/cron.ds 0640 /bin/bash
> apf.noarch: E: non-executable-script /etc/apf/internals/functions.apf 0640 /bin/bash
> apf.noarch: E: script-without-shebang /etc/apf/extras/.ca.def
> apf.noarch: E: script-without-shebang /etc/apf/vnet/vnetgen

This is just the way APF is written. 

> apf.noarch: E: non-readable /etc/apf/allow_hosts.rules 0640
> apf.noarch: E: non-readable /etc/apf/apf 0750
> apf.noarch: E: non-readable /etc/apf/bt.rules 0640
> apf.noarch: E: non-readable /etc/apf/conf.apf 0640
> apf.noarch: E: non-readable /etc/apf/deny_hosts.rules 0640
> apf.noarch: E: non-readable /etc/apf/ds_hosts.rules 0640
> apf.noarch: E: non-readable /etc/apf/ecnshame_hosts.rules 0640
> apf.noarch: E: non-readable /etc/apf/extras/dshield/cron.ds 0640
> apf.noarch: E: non-readable /etc/apf/extras/dshield/dshield-3.2.tar.gz 0640
> apf.noarch: E: non-readable /etc/apf/extras/dshield/install 0750
> apf.noarch: E: non-readable /etc/apf/extras/dshield/README 0640
> apf.noarch: E: non-readable /etc/apf/extras/get_ports 0750
> apf.noarch: E: non-readable /etc/apf/firewall 0750
> apf.noarch: E: non-readable /etc/apf/glob_allow.rules 0640
> apf.noarch: E: non-readable /etc/apf/glob_deny.rules 0640
> apf.noarch: E: non-readable /etc/apf/internals/compat.0.9.5 0640
> apf.noarch: E: non-readable /etc/apf/internals/cports.common 0640
> apf.noarch: E: non-readable /etc/apf/internals/functions.apf 0640
> apf.noarch: E: non-readable /etc/apf/internals/icmp.types 0640
> apf.noarch: E: non-readable /etc/apf/internals/internals.conf 0640
> apf.noarch: E: non-readable /etc/apf/internals/multicast.networks 0640
> apf.noarch: E: non-readable /etc/apf/internals/private.networks 0640
> apf.noarch: E: non-readable /etc/apf/internals/rab.ports 0640
> apf.noarch: E: non-readable /etc/apf/internals/reserved.networks 0640
> apf.noarch: E: non-readable /etc/apf/log.rules 0640
> apf.noarch: E: non-readable /etc/apf/main.rules 0640
> apf.noarch: E: non-readable /etc/apf/postroute.rules 0640
> apf.noarch: E: non-readable /etc/apf/preroute.rules 0640
> apf.noarch: E: non-readable /etc/apf/sdrop_hosts.rules 0640
> apf.noarch: E: non-readable /etc/apf/sysctl.rules 0640
> apf.noarch: E: non-readable /etc/apf/VERSION 0640
> apf.noarch: E: non-readable /etc/apf/vnet/main.vnet 0640
> apf.noarch: E: non-readable /etc/apf/vnet/vnetgen 0750
> apf.noarch: E: non-readable /etc/apf/vnet/vnetgen.def 0640
> apf.noarch: E: non-standard-dir-perm /etc/apf 0750

These are intentional so everyone on the system can't read the firewall rules. 

> apf.noarch: E: non-standard-executable-perm /etc/apf/apf 0750
> apf.noarch: E: non-standard-executable-perm /etc/apf/extras/dshield/install 0750
> apf.noarch: E: non-standard-executable-perm /etc/apf/extras/get_ports 0750
> apf.noarch: E: non-standard-executable-perm /etc/apf/firewall 0750
> apf.noarch: E: non-standard-executable-perm /etc/apf/vnet/vnetgen 0750
> apf.noarch: W: hidden-file-or-dir /etc/apf/extras/.ca.def
> apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/cron.ds
> apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/dshield-3.2.tar.gz
> apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/README
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/compat.0.9.5
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/cports.common
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/functions.apf
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/icmp.types
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/internals.conf
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/multicast.networks
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/private.networks
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/rab.ports
> apf.noarch: W: non-conffile-in-etc /etc/apf/internals/reserved.networks
> apf.noarch: W: non-conffile-in-etc /etc/apf/VERSION
> apf.noarch: W: non-conffile-in-etc /etc/apf/vnet/main.vnet
> apf.noarch: W: non-conffile-in-etc /etc/apf/vnet/vnetgen.def

See the comment about how how it stores everything in /etc/apf.

> apf.noarch: E: subsys-not-used /etc/init.d/apf

This is the way it is written. It doesn't have a daemon or PID so I can't create a lockfile for it. When you start or restart the service it runs its collection of shell scripts to create all the rules for iptables based on your config file, then exits while iptables continues to run.

> apf.noarch: E: zero-length /etc/apf/ds_hosts.rules
> apf.noarch: E: zero-length /etc/apf/ecnshame_hosts.rules
> apf.noarch: E: zero-length /etc/apf/sdrop_hosts.rules

Theses files do get used by APF.

> apf.noarch: W: non-conffile-in-etc /etc/logrotate.d/apf

I'm not sure why this is being marked.

> 2. I'd take out the BuildArch: noarch tag.

If I do this, rpmlint complains that it has no binary

> 3. add the %{?dist} tag to release.

Done.

> 4. Please don't chkconfig a service on by default: chkconfig --level 345 apf on

Fixed.

> 5. instead of defining basedir:

Fixed.

> 6. You can take out these two lines:

Fixed.

Spec URL: http://mmckinst.nexcess.net/apf/apf.spec
SRPM URL: http://mmckinst.nexcess.net/apf/apf-9.7.1-3.fc12.src.rpm

[nb@newharmony01 SPECS]$ rpmlint apf.spec 
apf.spec:16: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 16)
apf.spec: W: invalid-url Source0: apf-current.tar.gz
0 packages and 1 specfiles checked; 0 errors, 2 warnings.

[nb@newharmony01 SRPMS]$ rpmlint apf-9.7.1-3.fc12.src.rpm 
apf.src: W: spelling-error %description -l en_US iptables -> potables, portables, timetables
apf.src: W: spelling-error %description -l en_US netfilter -> net filter, net-filter, refilter
apf.src:16: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 16)
apf.src: W: invalid-url Source0: apf-current.tar.gz
1 packages and 0 specfiles checked; 0 errors, 4 warnings.

[nb@newharmony01 noarch]$ rpmlint apf-9.7.1-3.fc13.noarch.rpm 
apf.noarch: W: spelling-error %description -l en_US netfilter -> net filter, net-filter, refilter
apf.noarch: E: non-readable /etc/apf/allow_hosts.rules 0640L
apf.noarch: E: non-readable /etc/apf/internals/multicast.networks 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/multicast.networks
apf.noarch: E: non-readable /etc/apf/apf 0750L
apf.noarch: E: non-standard-executable-perm /etc/apf/apf 0750L
apf.noarch: E: non-readable /etc/apf/internals/private.networks 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/private.networks
apf.noarch: E: non-readable /etc/apf/VERSION 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/VERSION
apf.noarch: E: non-readable /etc/apf/extras/dshield/README 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/README
apf.noarch: E: non-readable /etc/apf/log.rules 0640L
apf.noarch: E: non-readable /etc/apf/bt.rules 0640L
apf.noarch: E: non-readable /etc/apf/internals/reserved.networks 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/reserved.networks
apf.noarch: E: non-readable /etc/apf/extras/dshield/install 0750L
apf.noarch: E: non-standard-executable-perm /etc/apf/extras/dshield/install 0750L
apf.noarch: E: non-readable /etc/apf/firewall 0750L
apf.noarch: E: non-standard-executable-perm /etc/apf/firewall 0750L
apf.noarch: W: non-conffile-in-etc /etc/logrotate.d/apf
apf.noarch: E: non-readable /etc/apf/conf.apf 0640L
apf.noarch: E: non-executable-script /etc/apf/conf.apf 0640L /bin/bash
apf.noarch: E: non-readable /etc/apf/vnet/vnetgen.def 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/vnet/vnetgen.def
apf.noarch: E: non-readable /etc/apf/deny_hosts.rules 0640L
apf.noarch: E: non-standard-dir-perm /etc/apf 0750L
apf.noarch: E: non-readable /etc/apf/ecnshame_hosts.rules 0640L
apf.noarch: E: zero-length /etc/apf/ecnshame_hosts.rules
apf.noarch: W: hidden-file-or-dir /etc/apf/extras/.ca.def
apf.noarch: E: script-without-shebang /etc/apf/extras/.ca.def
apf.noarch: E: non-readable /etc/apf/glob_deny.rules 0640L
apf.noarch: E: non-readable /etc/apf/ds_hosts.rules 0640L
apf.noarch: E: zero-length /etc/apf/ds_hosts.rules
apf.noarch: E: non-readable /etc/apf/extras/get_ports 0750L
apf.noarch: E: non-standard-executable-perm /etc/apf/extras/get_ports 0750L
apf.noarch: E: non-readable /etc/apf/internals/cports.common 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/cports.common
apf.noarch: E: non-readable /etc/apf/glob_allow.rules 0640L
apf.noarch: E: non-readable /etc/apf/main.rules 0640L
apf.noarch: E: non-readable /etc/apf/postroute.rules 0640L
apf.noarch: E: non-readable /etc/apf/extras/dshield/cron.ds 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/cron.ds
apf.noarch: E: non-executable-script /etc/apf/extras/dshield/cron.ds 0640L /bin/bash
apf.noarch: E: non-readable /etc/apf/sdrop_hosts.rules 0640L
apf.noarch: E: zero-length /etc/apf/sdrop_hosts.rules
apf.noarch: E: non-readable /etc/apf/vnet/vnetgen 0750L
apf.noarch: E: non-standard-executable-perm /etc/apf/vnet/vnetgen 0750L
apf.noarch: E: script-without-shebang /etc/apf/vnet/vnetgen
apf.noarch: E: non-readable /etc/apf/internals/compat.0.9.5 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/compat.0.9.5
apf.noarch: E: non-readable /etc/apf/internals/internals.conf 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/internals.conf
apf.noarch: E: non-readable /etc/apf/sysctl.rules 0640L
apf.noarch: E: non-readable /etc/apf/extras/dshield/dshield-3.2.tar.gz 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/extras/dshield/dshield-3.2.tar.gz
apf.noarch: E: non-readable /etc/apf/internals/icmp.types 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/icmp.types
apf.noarch: E: non-readable /etc/apf/internals/functions.apf 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/functions.apf
apf.noarch: E: non-executable-script /etc/apf/internals/functions.apf 0640L /bin/bash
apf.noarch: E: non-readable /etc/apf/vnet/main.vnet 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/vnet/main.vnet
apf.noarch: E: non-readable /etc/apf/preroute.rules 0640L
apf.noarch: E: non-readable /etc/apf/internals/rab.ports 0640L
apf.noarch: W: non-conffile-in-etc /etc/apf/internals/rab.ports
apf.noarch: W: no-manual-page-for-binary apf
apf.noarch: W: no-manual-page-for-binary fwmgr
apf.noarch: E: subsys-not-used /etc/init.d/apf
1 packages and 0 specfiles checked; 49 errors, 20 warnings.

More to come.

I don't believe most of those errors actually have to be fixed, but I'm researching this more

I've sponsored Mark, hence removing FE-NEEDSPONSOR.

I asked Nick on IRC and he indicated that he'd try to get back to this ticket.

Doesn't look like that ever happened.

Before I agitate some more, I should ask if Mark is still interested in submitting this package.

Just to make a useful comment, I'd say that several of those rpmlint complaints bear closer inspection.  The whole "put a pile of stuff under /etc/apf" doesn't seem very clean at all.  I know the (non systemd) init system puts executables there, and there's the whole /etc/sysconfig/network-scripts mess, but that doesn't mean that new packages should come along and drop a bunch of executables in /etc.  And documentation really doesn't seem to belong in there at all.

(In reply to comment #11)
> Doesn't look like that ever happened.
> 
> Before I agitate some more, I should ask if Mark is still interested in
> submitting this package.

I really don't have the time. The author has an unusual way of packaging his
stuff which doesn't fit well with Fedora. While its possible to change it around
and reduce the rpmlint complaints, I don't have the time necessary to do so.

> Just to make a useful comment, I'd say that several of those rpmlint complaints
> bear closer inspection.  The whole "put a pile of stuff under /etc/apf" doesn't
> seem very clean at all.  I know the (non systemd) init system puts executables
> there, and there's the whole /etc/sysconfig/network-scripts mess, but that
> doesn't mean that new packages should come along and drop a bunch of
> executables in /etc.  And documentation really doesn't seem to belong in there
> at all.

If anyone wants to pick this up, internals/internals.conf is where you'd have to
change some stuff but some custom patching will probably still be required. I'm
working on another of his packages that has another unusual layout, its
possible to make it fit in Fedora but its not easy.

No problem, I'll close this out.