Bug 463035 (pyroman)
Summary: | Review Request: pyroman - Very fast firewall configuration tool | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Adam Huffman <bloch> |
Component: | Package Review | Assignee: | 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: | rawhide | CC: | 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
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. 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. 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. Ping? Am working on another package which needs a partial rewrite - will get back to this one once that's done. what is the current status? Will you release a new version of the package? Setting NEEDINFO. Adam, ping? I've got an initscript for the package now but it needs more testing. Will upload a new version next week. ping, it's been more than a week :) 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 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 Ping? -- Fedora Bugzappers volunteer triage team https://fedoraproject.org/wiki/BugZappers As far as I can tell, the upstream project is inactive - no commits since August 2008. Wondering whether it's worth persisting? 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 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. Yes - I'd rather expend my energy on actively maintained packages. Thanks for the efforts of the reviewers. |