Good:
+ Naming seems ok.
+ Rpmlint quite on source rpm.
+ Local build works fine.
+ Mock build works fine.


Bad:
- Source0 contains not a full qualified URL.
- %{?_smp_mflags} missing on make without any comment.
- /usr/bin should be replace by %{_bindir}
- /usr/sbin should be replace by %{_sbindir}
- /ussr/share should be replace by %{_datadir}
You can owned a whole directory, if the entry in the %file stanza end with a slash
- Rpmlint complaints binary package:
W: gnome-network-monitor no-documentation
E: gnome-network-monitor script-without-shebang
/usr/share/gnome-network-monitor/gnm.glade
W: gnome-network-monitor conffile-without-noreplace-flag
/etc/pam.d/gnome-network-monitor
W: gnome-network-monitor conffile-without-noreplace-flag
/etc/security/console.apps/gnome-network-monitor
- Packages contains no docs.
- Package doesn't contain a verbatin copy of the license text
- Programm crashed after startup:
/usr/lib/python2.4/site-packages/gnome-network-monitor/gnm.py:69: GtkWarning:
gtk_widget_grab_default: assertion `GTK_WIDGET_CAN_DEFAULT (widget)' failed
  self.__xml_file     =  gtk.glade.XML(self.__glade_file, "gnome-network-monitor")
Updating information
Traceback (most recent call last):
  File "/usr/sbin/gnome-network-monitor", line 10, in ?
    gnm.run()
  File "/usr/lib/python2.4/site-packages/gnome-network-monitor/gnm.py", line
134, in run
    win = MainWindow()
  File "/usr/lib/python2.4/site-packages/gnome-network-monitor/gnm.py", line 97,
in __init__
    self.__iptables.parse_file()
  File "/usr/lib/python2.4/site-packages/gnome-network-monitor/iptables.py",
line 145, in parse_file
    self.__insert_record(FwRecord(line))       # store them in the db
  File "/usr/lib/python2.4/site-packages/gnome-network-monitor/iptables.py",
line 32, in __init__
    tup = time.strptime("%s %s %s"%(lfs[0], lfs[1], lfs[2]), "%b %d %H:%M:%S")
  File "/usr/lib64/python2.4/_strptime.py", line 293, in strptime
    raise ValueError("time data did not match format:  data=%s  fmt=%s" %
ValueError: time data did not match format:  data=May 7 19:08:04  fmt=%b %d %H:%M:%S

Jochen,
thanks for a quick response!

The updated packages are at:
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.9-1.src.rpm

1) These were packaging bugs and were fixed. Thanks for pointing them out!
>- Source0 contains not a full qualified URL.
>- /usr/bin should be replace by %{_bindir}
>- /usr/sbin should be replace by %{_sbindir}
>- /ussr/share should be replace by %{_datadir}
>- Rpmlint complaints binary package:
>- Packages contains no docs.

2) I'm not sure I completely understand these comments:
>- %{?_smp_mflags} missing on make without any comment.
Does this flag make sense for a pure python package? I haven't seen it with 
other similar programs (I was largely inspired by system-config-* and 
setroubleshooter's spec files)

> You can owned a whole directory, if the entry in the %file stanza end with a 
> slash

3)
>- Package doesn't contain a verbatin copy of the license text
Is this a requirement? The tarball contains license text. Not the RPM, but the 
specfile says it's GPL..is it really necessary to have a GPL text for every 
package on the system?

4)
>- Programm crashed after startup:
I filed your traceback as bug #1718208 at SF.net, it should be fixed now. Can 
you please re-try? Does it crash even if you select "Run unprivileged" from 
the usermode dialog

Again, thanks for jumping on this review!

I've updated the spec and the srpm:
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.9.1-2.src.rpm

God:
+ Local build works fine.
+ Local start works fine.
+ Local install/uninstall workd fine.
+ Rpmlint is quite on source rpm.
+ Ronlint is quite on binary rpm.
+ Mock build works fine.

Bad:
+ Tar ball doesn't matches with upstream.
+ Inconsistent use of buildroot
Use of buildroot is not consistant
  (wiki: Packaging/Guidelines#UsingBuildRootOptFlags)

Thanks for reviewing the package again. Both issues should be fixed now. The 
updated packages are located at:

http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.9.1-3.src.rpm

Good:
+ Tar ball matches with upstream.

Bad
- Local build fails:
This may coused by the most recent version of the desktop-files-utils from rawhide:
v
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/bin/gnome-network-monitor
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/sbin
ln -sf consolehelper
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/bin/gnome-network-monitor
+ desktop-file-install --vendor '' --delete-original --dir
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/share/applications
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/share/applications/gnome-network-monitor.desktop
/var/tmp/gnome-network-monitor-0.9.1-3.fc7-root-s4504kr/usr/share/applications/gnome-network-monitor.desktop:
error: file contains key "Category" in group "Desktop Entry", but keys extending
the format should start with "X-"
desktop-file-install created an invalid desktop file!

Yes, thanks for spotting that..the desktop file was broken wrt latest 
desktop-file-utils.

I fixed the desktop file, new packages are at:
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.9.1-4.fc8.src.rpm

Good:
+ Package meat naming guideline
+ Name of SPEC file meat base package name
* Package contains %{?dist} tag
+ Consistent usage of RPM macros
+ Package meets packaging guideline
+ License of the package is GPL
+ License field in SPEC file matches
+ SPEC is written in englisch
+ SPEC look legible
+ Tar ball in source RPM metaches with upstream
  md5sum: 5e9eb652e4e24e2057f6eb1b4794a683 
+ Package has correct buildroot
+ BuildRequires are not redundant
+ Package contains no sub packages
+ Local build works fine
+ SPEC file contains proper %defattr and %attr permission
+ Package has a correct %clean section
+ $RPM_BUILD_ROOT will be cleaned on start of %install section
+ Packages %doc section doesn't affect runtime.
+ %files section contains no duplicate files
+ Package contains no file or directories own by other packages.
+ Changelog section is correct.
+ Rpmlint is quite of source and binary rpm.
+ Mock build wors fine on Devel and F-7 (x86_64)

+ Package should contains the most recent version.
+ Startup of the package works without crash.

Bad:
- License file should be includes from tar ball.
- BR on gettext-devel may be better, because gettext-devel comtains
  development tools which are not in the gettext package.
- Wrong Requires:
  You use:
Requires(post):   /usr/bin/update-desktop-database
Requires(postun): /usr/bin/update-desktop-database

because you use desktop-file-install in your scriplets, you shoud
have the following in your Requires:

Requires(post):   /usr/bin/desktop-file-install
Requires(postun): /usr/bin/desktop-file-install

- Package does not sure the ownership of the directory 
   %{python_sitelib}/%{name}/

The best way to make sure, that the directory and all files belong to the
package is to write

%{python_sitelib}/%{name}/

into the SPEC file, so you can remove the line

%{python_sitelib}/%{name}/*.py*

from the SPEC file.

- Desktop entry contains no icon.

(In reply to comment #8)

> Requires(post):   /usr/bin/desktop-file-install
> Requires(postun): /usr/bin/desktop-file-install

Unfortunately, I have done a misktake.

Refering to
http://fedoraproject.org/wiki/PackagingDrafts/DesktopFiles?highlight=%28desktopfile%29

You should use

BuildRequires: desktop-file-install

instead.

Best Regards:

Jochen Schmitt

(In reply to comment #8)
Thank you for reviewing the package again, hope that everything will be OK 
this time :)

> Bad:
> - License file should be includes from tar ball.
Fixed, thanks for spotting that.

> - BR on gettext-devel may be better, because gettext-devel comtains
>   development tools which are not in the gettext package.
Why? I don't use anything from gettext-devel..remember, this is a python 
package. What exactly would be better if I included gettext-devel even though 
the package builds fine in mock chroot?


> - Wrong Requires:
>   You use:
> Requires(post):   /usr/bin/update-desktop-database
> Requires(postun): /usr/bin/update-desktop-database

Removed as spurious. Wrt your last comment and 
http://fedoraproject.org/wiki/PackagingDrafts/DesktopFiles I guess that only 
BuildRequires: desktop-file-install is needed.

> - Package does not sure the ownership of the directory 
>    %{python_sitelib}/%{name}/
> 
> The best way to make sure, that the directory and all files belong to the
> package is to write
> 
> %{python_sitelib}/%{name}/
> 
Thank you, fixed.

> 
> - Desktop entry contains no icon.
> 

I'm not sure what exactly do you mean...you don't see an icon in the menu? I 
just double checked in a vanilla F7 vmware install that I _can_ see the icon 
after installing the package..but I could reproduce the problem on another box 
with KDE as the only DE..I'm not entirely sure what the problem might be...

New packages located at:
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.9.1-5.fc8.src.rpm
http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec

(In reply to comment #10)

> Why? I don't use anything from gettext-devel..remember, this is a python 
> package. What exactly would be better if I included gettext-devel even though 
> the package builds fine in mock chroot?
> 

OK, you right. I was memorized on my own experience with one of my onw packages.

But because you wrote, that this is only a python package, this may be ok.
 

> I'm not sure what exactly do you mean...you don't see an icon in the menu? I 
> just double checked in a vanilla F7 vmware install that I _can_ see the icon 
> after installing the package..but I could reproduce the problem on another box 
> with KDE as the only DE..I'm not entirely sure what the problem might be...

I don't see the icon on my KDE Desktop.

Best Regards: 

Jochen Schmitt

I have took a further look to your package.

The reported issue with the menu icon is caused by the missing icon file in your
package.

Ping Jakub

Pong :-) 
I'm sorry for not being responsive, I was on vacation for the last two weeks 
and I'm still somehow catching up (I should probably have updated a related 
wiki page..). I'll return to this review later this week. 

Ping

Bacause I didn't see any action for the last 5 month, I may close this bug.