[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=225906 Kevin Fenzi changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution||RAWHIDE Flag|fedora-review? |fedora-review+ --- Comment #14 from Kevin Fenzi 2009-03-07 01:25:45 EDT --- Excellent. I don't see any further blockers, so this package is APPROVED. Thanks for fixing things up. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225906
--- Comment #13 from Thomas Woerner 2009-03-05 09:14:11
EDT ---
Please have a look at iptables-1.4.2-3 in rawhide.
Solved Issues:
1) using %{buildroot} everywhere
3) using sed instead of perl
4) #462207 has already been addressed in iptables-1.4.2-1
#432617 will require basic firewall changes and is therefore not possible
now
5) using RPM_OPT_FLAGS, but with -fno-strict-aliasing (needed for libiptc)
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=225906 --- Comment #12 from Kevin Fenzi 2009-02-22 20:50:55 EDT --- Oh. I also just noticed that this package doesn't seem to honor the standard rpm CFLAGS. Is there any reason for that? -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=225906
--- Comment #11 from Kevin Fenzi 2009-02-22 13:52:14 EDT ---
I went ahead and ran through my checklist again:
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
See below - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL+)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
a138d1c2e74321e0e4e228a9fb301c9a iptables-1.4.2.tar.bz2
a138d1c2e74321e0e4e228a9fb301c9a iptables-1.4.2.tar.bz2.orig
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install
OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
OK - .pc files in -devel subpackage/requires pkgconfig
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - Package obey's FHS standard (except for 2 exceptions)
See below - No rpmlint output.
OK - final provides and requires are sane.
SHOULD Items:
OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
OK - Should package latest version
OK - Should not use file requires outside of /etc, /bin, /sbin, /usr/bin, or
/usr/sbin
See below - check for outstanding bugs on package (merge
reviews/rename/re-reviews).
Issues:
1. MINOR: Can you stick with one of $RPM_BUILD_ROOT or %{buildroot} ?
2. rpmlint says:
iptables.src: W: strange-permission iptables.init 0755
iptables.x86_64: E: non-readable /etc/sysconfig/iptables-config 0600
iptables.x86_64: W: service-default-enabled /etc/rc.d/init.d/iptables
iptables.x86_64: W: service-default-enabled /etc/rc.d/init.d/iptables
iptables-ipv6.x86_64: E: non-readable /etc/sysconfig/ip6tables-config 0600
iptables-ipv6.x86_64: W: service-default-enabled /etc/rc.d/init.d/ip6tables
iptables-ipv6.x86_64: W: service-default-enabled /etc/rc.d/init.d/ip6tables
iptables-ipv6.x86_64: W: incoherent-init-script-name ip6tables
All those can be ignored.
3. You might consider using one of the standard ways to remove rpath:
http://fedoraproject.org/wiki/Packaging/Guidelines (ie, sed instead of perl)
4. There are a few packaging related bugs that would be nice to fix up:
https://bugzilla.redhat.com/show_bug.cgi?id=462207
https://bugzilla.redhat.com/show_bug.cgi?id=432617
The consistent use of macros is a MUST, so if you can fix that up I can
approve this review.
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=225906 --- Comment #10 from Thomas Woerner 2009-02-20 09:17:47 EDT --- Please have a look at iptables-1.4.2-1 in rawhide. 1) License is GPL+, now 2) The old libraries are still static, the new ones are shared. Making the old ones static, could break build scripts and the old libraries should be gone in the near future. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=225906 --- Comment #9 from Kevin Fenzi 2009-01-14 01:06:15 EDT --- Any news here? Since it's been so long I would be happy to run through my checklist again against the current package... Or if you would care to look and address those last 2 items, I can look after that. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.
Summary: Merge Review: iptables
https://bugzilla.redhat.com/show_bug.cgi?id=225906
--- Additional Comments From [EMAIL PROTECTED] 2008-04-15 22:04 EST ---
Sorry for the delay here. Looking much better now:
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
See below - License
See below - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
90cfa8a554a29b0b859a625e701af2a7 iptables-1.4.0.tar.bz2
90cfa8a554a29b0b859a625e701af2a7 iptables-1.4.0.tar.bz2.orig
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Headers/static libs in -devel subpackage.
See below - Spec has needed ldconfig in post and postun
See below - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:
SHOULD Items:
OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
OK - Should package latest version
Issues:
1. You have the License as GPLv2.
I see a mix in the source files of: GPL, GPLv2, GPLv2+, GPLv2 or GPLv3.
I think this results in: GPL+ for the license?
2. Any reason the package makes a static lib instead of a shared lib?
Does anything use iptables-devel? Might be nice to remove the .a and
make a shared lib instead.
3. rpmlint says:
iptables.src: W: strange-permission iptables.init 0755
iptables.x86_64: E: non-readable /etc/sysconfig/iptables-config 0600
iptables.x86_64: W: service-default-enabled /etc/rc.d/init.d/iptables
iptables.x86_64: W: service-default-enabled /etc/rc.d/init.d/iptables
iptables-ipv6.x86_64: E: non-readable /etc/sysconfig/ip6tables-config 0600
iptables-ipv6.x86_64: W: service-default-enabled /etc/rc.d/init.d/ip6tables
iptables-ipv6.x86_64: W: incoherent-init-script-name ip6tables
All look ignorable.
So, items 1 and 2 look to be the last issues to address... thoughts?
--
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
--- You are receiving this mail because: ---
You are the QA contact for the bug, or are watching the QA contact.
___
Fedora-package-review mailing list
Fedora-package-review@redhat.com
http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/show_bug.cgi?id=225906 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEEDINFO|ASSIGNED Flag|needinfo?([EMAIL PROTECTED]| |m) | --- Additional Comments From [EMAIL PROTECTED] 2008-03-24 17:08 EST --- I think I forgot to report back to this bugzilla. Please have a look at the latest package in rawhide (1.4.0-X). I have made lots of changes because of the review in version 1.3.8-2, already. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/show_bug.cgi?id=225906 [EMAIL PROTECTED] changed: What|Removed |Added Version|devel |rawhide --- Additional Comments From [EMAIL PROTECTED] 2008-03-21 21:08 EST --- Hey Thomas, any chance we could move this forward? Or have you made changes already that you would like me to check here... -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225906 --- Additional Comments From [EMAIL PROTECTED] 2007-08-23 10:31 EST --- *** Bug 238791 has been marked as a duplicate of this bug. *** -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225906 [EMAIL PROTECTED] changed: What|Removed |Added Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora --- Additional Comments From [EMAIL PROTECTED] 2007-07-25 16:14 EST --- (In reply to comment #2) > 10. 21 outstanding bugs. None appear to be directly packaging related. Bug 216733 is packaging related, if you count files in the tarball that should be installed by the rpm a packaging problem (I would). -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225906 [EMAIL PROTECTED] changed: What|Removed |Added Status|ASSIGNED|NEEDINFO Flag|fedora-review- |needinfo?([EMAIL PROTECTED] ||m), fedora-review? --- Additional Comments From [EMAIL PROTECTED] 2007-02-23 21:53 EST --- Setting the review flag to ? here, since this is under review. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.
Summary: Merge Review: iptables
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225906
[EMAIL PROTECTED] changed:
What|Removed |Added
Flag|fedora-review? |fedora-review-
--- Additional Comments From [EMAIL PROTECTED] 2007-02-13 22:40 EST ---
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
dd965bdacbb86ce2a6498829fddda6b7 iptables-1.3.7.tar.bz2
dd965bdacbb86ce2a6498829fddda6b7 iptables-1.3.7.tar.bz2.1
See below - BuildRequires correct
See below - Package is relocatable and has a reason to be.
See below - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Headers/static libs in -devel subpackage.
See below - Spec has needed ldconfig in post and postun
See below - .so files in -devel subpackage.
See below - -devel package Requires: %{name} = %{version}-%{release}
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
OK - final provides and requires are sane:
SHOULD Items:
OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
See below - Should have subpackages require base package with fully versioned
depend.
See below - Should have dist tag
OK - Should package latest version
21 outstanding bugs - check for outstanding bugs on package.
Issues:
1. The Source URL is no longer correct.
Suggest: http://www.netfilter.org/projects/iptables/files/iptables-1.3.7.tar.bz2
2. Can the
Prefix: %{_prefix}
be removed? Is there any reason this package needs to be relocatable?
3. Minor: The BuildRequires: /usr/bin/perl
isn't required, as perl is part of the default build root.
4. Is the %defattr(-,root,root,0755) needed? or will %defattr(-,root,root,-)
work?
5. Use the default correct buildroot:
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
6. Any reason the package makes a static lib instead of a shared lib?
Does anything use iptables-devel? Might be nice to remove the .a and
make a shared lib instead.
7. The devel and ipv6 subpackages have
Requires: %{name} = %{version}
Suggest: change that to:
Requires: %{name} = %{version}-%{release}
8. Our buddy rpmlint says:
a)
W: iptables summary-ended-with-dot Tools for managing Linux kernel packet
filtering capabilities.
Suggest: remove . at end of summary.
b)
E: iptables tag-not-utf8 %changelog
Looks like the checkins from
Trond Eivind Glomsrd <[EMAIL PROTECTED]>
are not proper ut8f.
Suggest: Change to utf8.
c)
W: iptables strange-permission iptables.init 0755
Suggest: change the source file permissions to 644 and install it
as 755 only when installing?
d)
E: iptables non-utf8-spec-file iptables.spec
The encoding of the entire spec is not utf8.
Suggest: run iconv on it?
e)
E: iptables broken-syntax-in-scriptlet-requires Requires(post,postun): chkconfig
There was a rpm bug that made that syntax not work right.
Suggest: Change to:
Requires(post): /sbin/chkconfig
Requires(preun): /sbin/chkconfig
f)
W: iptables redundant-prefix-tag
Suggest: remove prefix.
g)
W: iptables rpm-buildroot-usage %prep rm -rf %{buildroot}
Suggest: remove rm in prep stage. It's not needed here.
h)
W: iptables macro-in-%changelog postun
W: iptables macro-in-%changelog preun
Suggest: Change any %macro names in changelog to be %%macro so rpm
doesn't try and expand them.
i)
E: iptables no-cleaning-of-buildroot %install
Suggest: add the rm -rf $RPM_BUILD_ROOT here.
j)
W: iptables conffile-without-noreplace-flag /etc/rc.d/init.d/iptables
Suggest: No need to mark the init script as a config file.
k)
E: iptables non-readable /etc/sysconfig/iptables-config 0600
I guess this can be ignored. Not sure how much security it provides however.
l)
W: iptables service-default-enabled /etc/rc.d/init.d/iptables
Normally this is a no-no, but in this case I think we do want it
enabled by default.
m)
W: iptables no-reload-entry /etc/rc.d/init.d/iptables
reload doesn't make sense here.
n)
W: iptables-devel spurious-executable-perm /usr/include/iptables.h
W: iptables-devel spurious-executable-perm /usr/include/ip6tables.h
W: iptables-devel spurious-executable-perm /usr/i
[Bug 225906] Merge Review: iptables
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Merge Review: iptables https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225906 [EMAIL PROTECTED] changed: What|Removed |Added Status|NEW |ASSIGNED AssignedTo|[EMAIL PROTECTED]|[EMAIL PROTECTED] Flag||fedora-review? --- Additional Comments From [EMAIL PROTECTED] 2007-02-13 21:59 EST --- I'd be happy to review this package. Look for a full review in a bit. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the QA contact for the bug, or are watching the QA contact. ___ Fedora-package-review mailing list Fedora-package-review@redhat.com http://www.redhat.com/mailman/listinfo/fedora-package-review
