Bug 239435 - Review Request: Gnome Network Monitor - A Network Monitor for the GNOME Desktop
Review Request: Gnome Network Monitor - A Network Monitor for the GNOME Desktop
Status: CLOSED CANTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-08 09:15 EDT by Jakub Hrozek
Modified: 2008-08-14 09:48 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-06-08 15:00:42 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Jakub Hrozek 2007-05-08 09:15:16 EDT
Spec URL: http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor.spec
SRPM URL: http://gnetworkmonitor.sourceforge.net/rpm/gnome-network-monitor-0.8-1.src.rpm

Description:
Gnome Network Monitor is a tool that allows monitoring of network activity on
a system.
Features:
* Network statistics overview
* Per-process overview of network traffic on your computer visually similar to
  top
* Bandwidth usage with graphical histogram and other information about network
  interfaces such as IP address
* Parsing of iptables log
Comment 1 Jochen Schmitt 2007-05-08 12:11:01 EDT
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




Comment 2 Jakub Hrozek 2007-05-14 05:19:46 EDT
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!
Comment 4 Jochen Schmitt 2007-05-22 11:39:43 EDT
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)

Comment 5 Jakub Hrozek 2007-05-24 08:11:52 EDT
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
Comment 6 Jochen Schmitt 2007-06-21 14:07:09 EDT
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!


Comment 7 Jakub Hrozek 2007-06-26 12:35:05 EDT
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
Comment 8 Jochen Schmitt 2007-06-26 13:48:44 EDT
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.




Comment 9 Jochen Schmitt 2007-06-27 10:03:28 EDT
(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

Comment 10 Jakub Hrozek 2007-06-27 11:12:07 EDT
(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
Comment 11 Jochen Schmitt 2007-06-27 11:19:10 EDT
(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
Comment 12 Jochen Schmitt 2007-07-10 13:52:22 EDT
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.
Comment 13 Jochen Schmitt 2007-07-26 12:02:37 EDT
Ping Jakub
Comment 14 Jakub Hrozek 2007-07-26 12:14:24 EDT
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. 
Comment 15 Jochen Schmitt 2008-01-08 15:21:13 EST
Ping
Comment 16 Jochen Schmitt 2008-06-08 15:00:42 EDT
Bacause I didn't see any action for the last 5 month, I may close this bug.

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