Bug 463035 (pyroman)

Summary: Review Request: pyroman - Very fast firewall configuration tool
Product: [Fedora] Fedora Reporter: Adam Huffman <bloch>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, gratien.dhaese, mtasaka, notting, oget.fedora, thomasj, tuju
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
URL: http://pyroman.alioth.debian.org/
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-11-08 19:55:35 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Adam Huffman 2008-09-20 23:49:50 UTC
Spec URL: http://verdurin.fedorapeople.org/review/pyroman/pyroman.spec
SRPM URL: http://verdurin.fedorapeople.org/review/pyroman/pyroman-0.4.6-1.fc10.src.rpm
Description: Pyroman is a firewall tool written in Python for complex networks, 
but it can of course also handle simple single-host-single-link setups.

Pyroman is inspired by Shorewall and FireHOL, but tries to improve upon them
with respect to performance and ease of configuration.

Pyroman currently only configures IPv4 iptables/netfilter firewalls, it does
not include configuration utilities for setting up VPN or traffic shaping.

Comment 1 Orcan Ogetbil 2008-10-07 00:32:13 UTC
Hi, I just went through your package. It needs some work. Here are my notes:
-------------------------------------------------------------------------
The summary needs to be beefed up and more informative IMHO.
-------------------------------------------------------------------------
When I run the "pyroman" script I get the error:
No rule files found in directory './examples/base'!
I think you need to change the default_rules_path in the launching script (to %{_sysconfdir}/%name via 'sed', for instance).
---------------------------------------------------------------
In the %files section, we prefer to use 
   %defattr(-, root, root, -)
Also
   %attr(0644, root, root)
are redundant. Instead you should use
   install -pm 644 pyroman/* $RPM_BUILD_ROOT/%{python_sitelib}/%{name}/
and
   install -Dpm 644 doc/pyroman.8 $RPM_BUILD_ROOT/%{_mandir}/man8/pyroman.8
in the %install section. Also make sure you use the switch -p of the "install" command to preserve the timestamps whenever it makes sense (usually for the non-compiled files).
-------------------------------------------------------------------------
License is MIT, not GPL
-------------------------------------------------------------------------
   BuildRequires:	python-devel
is redundant. Package will build without it.
   Requires:		python
is not necessary either. rpmbuild will pick that up.
-------------------------------------------------------------------------
You can further replace
   dir %{python_sitelib}/%{name}/
   %{python_sitelib}/%{name}/*.py*
with 
   %{python_sitelib}/%{name}
to improve the spec file.
-------------------------------------------------------------------------
Make sure you make use of %{name} and %{version} macros in addition to the predefined directory macros.
For instance, you use
   install -D bin/pyroman $RPM_BUILD_ROOT/usr/sbin/pyroman
in one line but
   %{_sbindir}/%{name}
on the other, which is inconsistent and not desired.

Comment 2 Adam Huffman 2008-10-08 23:37:59 UTC
Thanks for having a look.  I've addressed most of your comments, though not yet the problem with the rulesets.  I need to think about how best to handle both the built-in rules and those the user creates.  Probably I'll adapt what's in the Debian package, as noted in https://bugzilla.redhat.com/show_bug.cgi?id=454220#c36

New versions at:

http://verdurin.fedorapeople.org/review/pyroman/pyroman.spec

http://verdurin.fedorapeople.org/review/pyroman/pyroman-0.4.6-2.fc10.src.rpm

Once I've a working fix for the rule handling, I'll upload a new version.

Comment 3 Orcan Ogetbil 2008-10-10 03:38:33 UTC
Thank you for the update. The package is in better shape now.
Just figure out what to do with the rules files and the package will be good to go.

Comment 4 Orcan Ogetbil 2008-11-13 21:08:26 UTC
Ping?

Comment 5 Adam Huffman 2008-11-16 20:08:48 UTC
Am working on another package which needs a partial rewrite - will get back to this one once that's done.

Comment 6 Gratien D'haese 2009-01-23 09:06:59 UTC
what is the current status? Will you release a new version of the package?

Comment 7 Mamoru TASAKA 2009-01-24 16:05:01 UTC
Setting NEEDINFO. Adam, ping?

Comment 8 Adam Huffman 2009-01-25 22:33:21 UTC
I've got an initscript for the package now but it needs more testing.  Will upload a new version next week.

Comment 9 Susi Lehtola 2009-03-20 19:53:51 UTC
ping, it's been more than a week :)

Comment 10 Adam Huffman 2009-03-23 16:05:48 UTC
That's very true.  I've put up a new version with my first attempt at an initscript - comments welcome as there's probably something wrong with it.

http://verdurin.fedorapeople.org/review/pyroman/pyroman-0.4.6-3.fc10.src.rpm
http://verdurin.fedorapeople.org/review/pyroman/pyroman.spec

Comment 11 Orcan Ogetbil 2009-04-04 05:30:36 UTC
Phew, it's been 6 months. This was one of my first reviews and I barely remember it.

Anyway, since then, there were some changes in the guidelines too. I had a quick look at the package:

* The problem that I reported before remains:
  When I run the "pyroman" script I get the error:
      No rule files found in directory './examples/base'!
  I think you need to change the default_rules_path in the launching script (to
  %{_sysconfdir}/%name via 'sed', for instance).

* Please make use of the %{name} macro in %install

* Please use the -p switch of install extensively, to preserve timestamps.

* %setup -q -n %{name}-%{version}
is not necessary. 
  %setup -q
should be enough

* We certainly don't want
  %define _unpackaged_files_terminate_build 0

* The new guidelines suggest that, In the first line, %define should be replaced by %global

? Where does SOURCE1 come from? Why the name fedora.init ?

! You probably don't need
  cp -p %SOURCE1 .
because you can use
  install -pm 0755 %{SOURCE1} %{buildroot}/%{_initrddir}/%{name}
in the %install section.

* Also there are two rpmlints that deserve attention:
   pyroman.noarch: W: service-default-enabled /etc/rc.d/init.d/pyroman
   pyroman.noarch: W: incoherent-subsys /etc/rc.d/init.d/pyroman $prog

Comment 12 Thomas Janssen 2009-10-04 09:45:00 UTC
Ping?

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 13 Adam Huffman 2009-10-04 10:46:24 UTC
As far as I can tell, the upstream project is inactive - no commits since August 2008.  Wondering whether it's worth persisting?

Comment 14 Thomas Janssen 2009-10-07 10:06:31 UTC
If you still want it, you could try to ask upstream if they're still alive :) If they gave up you could become upstream ;) Question is, is it worth.
But that's up to you.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 15 Jason Tibbitts 2009-11-08 19:01:28 UTC
Was there ever a decision as to whether this ticket would be pursued?  It's been a month since comment #14, so I guess I'll go ahead and close this soon if there's no further activity.

Comment 16 Adam Huffman 2009-11-08 19:55:35 UTC
Yes - I'd rather expend my energy on actively maintained packages.  Thanks for the efforts of the reviewers.