Bug 592864 - Review Request: kmyfirewall - IPTables Based Firewall
Summary: Review Request: kmyfirewall - IPTables Based Firewall
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2010-05-17 08:57 UTC by Aditya Patawari
Modified: 2010-11-15 18:36 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-11-15 18:36:33 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Aditya Patawari 2010-05-17 08:57:07 UTC
Spec URL: http://adimania.fedorapeople.org/specs/kmyfirewall.spec
SRPM URL: http://adimania.fedorapeople.org/src.rpms/kmyfirewall-1.1.1-1.fc12.src.rpm
Description: KMyFirewall attempts to make it easier to setup IPTables based firewalls on Linux systems.
Koji Scratch Build URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2191747

Comment 1 Parag AN(पराग) 2010-05-17 09:35:46 UTC
As you are not sponsored yet so its always recommended to add FE-NEEDSPONSOR to all your review requests.

Comment 2 Gowrishankar Rajaiyan 2010-05-29 06:40:52 UTC
Pre-review:

* Doesn't build, please fix the following:
checking for Qt... configure: error: Qt (>= Qt 3.2 and < 4.0) (library qt-mt) not found. Please check your installation!
For more details about this problem, look at the end of config.log.

rpm -q qt
qt-4.6.2-17.fc12.x86_64
-------------------------------------------------------------------------------


Once you fix the above please check the following too:

* Using %name / %version macro in SourceURL
  - Using %name / %version (especially %version) in SourceURL
    is generally useful:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D

* %{_datadir}/icons/hicolor
contains the directory %_datadir/icons/hicolor and all files/directories/etc under this directory. 

* %{_datadir}/icons/Locolor
contains the directory %_datadir/icons/hicolor and all files/directories/etc under this directory.

* If "BuildArch: i686" only, then
  http://fedoraproject.org/wiki/Packaging/Guidelines#Architecture_Build_Failures

Comment 3 Mamoru TASAKA 2010-05-29 16:59:54 UTC
Well, this package "builds" at least on F-14 and F-13.
Some comments from me

- For sourceforge hosted packages, please check
  https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

- Some BuildRequires are redundant. For example, zlib-devel always
  require zlib, so with "BR: zlib-devel", "BR: zlib" is not needed.

  Also kdelibs3-devel requires qt3-devel, so BR: qt3-devel is redundant.

- For Fedora BuildRoot tag is no longer needed (EPEL build still
  needs this)
  https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

- Please explain why you restrict BuildArch to i686. If build
  really fails on x86_64 please try to fix it.

- Currently Fedora specific compilation flags are not honored
  and generated debuginfo rpm is incomplete.
  Please make it sure that Fedora specific compilation flags are
  honored correctly.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags

- There are some %files list mistakes on directory ownership issue.
  For example, the directory %{_datadir}/apps/%{name}/ itself
  is not owned by any packages.
  Please check the following wiki page and make it sure that all
  directories which are created newly by installing this pacakges
  are owned by this package:
  https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes

  ! Note
    This package need not own the directory %{_datadir}/icons/ directories
    or so themselves, so %files entry
------------------------------------------------------------------
%{_datadir}/icons/hicolor/16x16/apps/%{name}.png
%{_datadir}/icons/hicolor/22x22/apps/%{name}.png
.....
------------------------------------------------------------------
    are correct (however using wildcard will make this part more
    readable)

- It seems that files under %{_datadir}/icons/Locolor should be moved
  to under {_datadir}/icons/locolor (the latter directory is owned by
  kde-filesystem)

- "make clean" in %clean stage is unneeded as the directory
  where tarball is expanded is anyway deleted.
  Also on Fedora 13 and above, writing %clean section is completely
  unneeded:
  https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean

- Usually "INSTALL" file is for people trying to compile and install
  softwares by themselves and not needed for people using rpm (or
  other package manager)

- The rebuilt binary contains rpaths (you can check this by
  using "rpmlint" command (in rpmlint rpm). Please check your srpm /
  rebuilt binary rpms / installed rpm by rpmlint).
-------------------------------------------------------------------
kmyfirewall.i686: E: binary-or-shlib-defines-rpath /usr/lib/libkmfcore.so.0.0.0 ['/usr/lib', '/usr/lib/qt-3.3/lib']
kmyfirewall.i686: E: binary-or-shlib-defines-rpath /usr/lib/kde3/libkmfruletargetoptionedit_tos.so ['/usr/lib', '/usr/lib/qt-3.3/lib']
kmyfirewall.i686: E: binary-or-shlib-defines-rpath /usr/bin/kmyfirewall ['/usr/lib', '/usr/lib/qt-3.3/lib']
kmyfirewall.i686: E: binary-or-shlib-defines-rpath /usr/lib/kde3/libkmfruleoptionedit_mac.so ['/usr/lib', '/usr/lib/qt-3.3/lib']
......
-------------------------------------------------------------------
  And this type of rpaths are not allowed on Fedora. Please remove rpaths
  on these binaries:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath

  Note using "chrpath" binary for removing rpaths must be regarded
  as a "last resort".

- Please explain why header files under %_includedir/%name are needed for
  runtime. These files are usually needed only for building software and
  are not needed for runtime.
  https://fedoraproject.org/wiki/Packaging/Guidelines#Devel_Packages

  Also %_libdir/libkmfwidgets.so is usually for development files and
  not needed for runtime.

- .la libtool files or .a static archives should be removed unless
  some reason exists:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries

- Files under %_syscondir should usually be marked as %config(noreplace),
  ref:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files

- Please include some needed scriptlets
  - /sbin/ldconfig must be called for %_libdir/lib***.so.* :
    https://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

  - GTK icon cache must be updated:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

  - As the desktop under %_datadir/applications/kde contains Mimetype key,
    desktop database must be updated:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#desktop-database

Comment 4 Mamoru TASAKA 2010-06-06 16:59:09 UTC
ping?

Comment 5 Aditya Patawari 2010-06-06 17:26:28 UTC
hi,
It might take some time for me to correct all of this. I am right now involved with Fedora Summer Coding and another package but rest assured I'll fix all the issues and get back to you as soon as I can. Thanks for review :)

Comment 6 Mamoru TASAKA 2010-11-05 17:35:39 UTC
Again ping?

Comment 7 Mamoru TASAKA 2010-11-15 16:45:53 UTC
ping again?

Comment 8 Aditya Patawari 2010-11-15 16:54:49 UTC
Hi,

First off, I am sorry for my unresponsive behavior. It was largely because I was trying to contact upstream regarding some fixes but all in vain.
The project has been dead for around 3 years and I don't see any point in packaging it now. Therefore I am not interesting in this anymore.

Sorry again for all the trouble.

Comment 9 Mamoru TASAKA 2010-11-15 18:36:33 UTC
Hello.

Thank you for replying anyway. Now I close this bug. If someone
wants to import this package into Fedora, please open a new review
request and mark this one as a duplicate of the new one,
thank you.


Note You need to log in before you can comment on or make changes to this bug.