Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 225604 - Review Request: etherape - Graphical network monitor for Unix
Summary: Review Request: etherape - Graphical network monitor for Unix
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: manuel wolfshant
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2007-01-31 17:23 UTC by Michael Rice
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-02-14 04:06:37 UTC
Type: ---
Embargoed:
wolfy: fedora-review+
notting: fedora-cvs+


Attachments (Terms of Use)

Description Michael Rice 2007-01-31 17:23:53 UTC
SPEC URL: http://fluxbox-wiki.org/packages/fedora/etherape.spec
SRPM URL: http://fluxbox-wiki.org/packages/fedora/etherape-0.9.7-1.src.rpm
Description: 
EtherApe is a graphical network monitor for Unix modeled after etherman.

No noise from rpmlint, I dont have mock setup right now so I was unable to test the mock build.

Comment 1 manuel wolfshant 2007-01-31 18:49:20 UTC
First notice after a quick glimpse
- you are using both $RPM_BUILD_ROOT and %{buildroot}. You should settle to
either one, but use only that one
- glibc-headers is on the exception list, no need to buildrequire it
- you have some duplicates in buildrequires: libglade2-devel pulls gtk2-devel
which in turn pulls glib2-devel
- you have some missing build-requires: libgnomeui-devel scrollkeeper
After those additions it will build in mock.

Please fix the above and I will do a full review

Comment 2 Michael Rice 2007-01-31 21:41:45 UTC
Thanks for checking it in mock. I hope to have my mock enviroment set back up soon.


http://fluxbox-wiki.org/packages/fedora/etherape.spec
http://fluxbox-wiki.org/packages/fedora/etherape-0.9.7-2.src.rpm

Comment 3 manuel wolfshant 2007-02-01 10:44:35 UTC
Good
- package meets naming guidelines
- package meets packaging guidelines
- license is GPL, text in %doc, matches source, includes COPYING from the sources
- spec file legible, in am. english
- source matches upstream, is last available version, sha1sum
72e5e570530a89ea962a17e55723318010e9a8e5  etherape-0.9.7.tar.gz
- package compiles on devel (x86) and FC6
- no missing BR
- MINOR: unnecessary BR libglade-devel (brought in by libgnomeui-devel)
- handles locales properly
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- docs are small, but part of them are in scrollkeeper format (see below)
- nothing in %doc affects runtime
- no scriptlets
- no static content
- no pkgconfig(.pc) files
- no libtool archives


SHOULD
- builds in mock on all tested platforms (FC6, devel, i386, x86_64)
- runs OK on FC6/x86_64

MUSTFIX:
- takes ownership of /usr/share/omf and /usr/share/pixmaps
quire scrollkeeper
- the desktop file is installed using --add-category X-Fedora which is no longer
required (see PackagingGuidelines#desktop in the wiki)
- part of the documentation requires scrollkeeper. I suggest one of the
following two approaches
-- either add scrollkeeper to Requires: or
-- build a separate package for docs and make that package require scrollkeeper

And a question: any other reason for not including the HTML pages except for the
fact that in the source tarball they are available in both text and HTML format ?


Comment 4 Patrice Dumas 2007-02-01 14:24:19 UTC

> - part of the documentation requires scrollkeeper. 

Only at build/install time. (and there is the issue of /usr/share/omf
ownership).

The scrollkeeper scriptlets are missing, however, see:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-a4ea5e1946bc113d19d24b4f5bfb543c579e5fc8

In my opinion having scrollkeeper as a dependency for a gnome 
based package is not an issue at all. It is better, in my opinion,
to have the documentation available in gnome-help in the main package.
In fact, in general the documentation is accessible from the application
window. It is not the case for etherape, but it could become the case.
What is debatable is whether or not to have requires for yelp.
I think it is not strictly necessary, but opinions may differ.

Comment 5 Michael Rice 2007-02-10 14:46:21 UTC
I couldnt find anything about yelp in the packaging guidelines so I didnt add
anything for it.

http://fluxbox-wiki.org/packages/fedora/etherape-0.9.7-3.src.rpm

Comment 6 Patrice Dumas 2007-02-10 15:22:59 UTC
It is fortunate that there is nothing about yelp in the guidelines.
It is a viewer for the documentation handled by scrollkeeper.

Maybe you could flag as %doc everything that is below
%{_datadir}/%{name}/doc/
And also maybe 
%{_datadir}/omf/etherape/etherape-C.omf

The day it becomes possible to view the doc from the etherape window
you would have to remove the %doc qualifier, but today it doesn't
seems to be used in runtime (except if I missed something).

I also suggest removing 'for UNIX' from the description.

Comment 7 Patrice Dumas 2007-02-10 15:54:55 UTC
(In reply to comment #3)

> - takes ownership of /usr/share/omf and /usr/share/pixmaps

/usr/share/pixmaps is already owned by filesystem. 

Comment 8 manuel wolfshant 2007-02-10 21:53:02 UTC
(in reply to comment #7)
That's why I flagged as an error the presence of /usr/share/pixmaps in %files

Comment 9 manuel wolfshant 2007-02-10 22:37:26 UTC
Almost everything seems fine now. except with the desktop file: according to
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755
"If upstream uses <vendor_id>, leave it intact, otherwise use fedora as
<vendor_id>." Therefore you should use desktop-file-install --vendor="fedora". 

Good
- package meets naming guidelines
- package meets packaging guidelines
- license is GPL, text in %doc, matches source, includes COPYING from the sources
- spec file legible, in am. english
- source matches upstream, is last available version, sha1sum
72e5e570530a89ea962a17e55723318010e9a8e5  etherape-0.9.7.tar.gz
- package compiles on devel (x86_64)
- no missing BR
- MINOR (not a blocker): unnecessary BR libglade2-devel (brought in by
libgnomeui-devel)
- handles locales properly
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- docs are small
- nothing in %doc affects runtime
- scriptlets respect packaging recommendations
- no static content, pkgconfig(.pc) files, or libtool archives

All problems mentioned in comments #3 and #4 are fixed. The package is APPROVED
but please fix the desktop-file install command before importing. You could also
remove libglade2-devel from BR, unless you have an important reason to keep it
mentioned.


Comment 10 Patrice Dumas 2007-02-11 10:29:24 UTC
(In reply to comment #9)
> You could also
> remove libglade2-devel from BR, unless you have an important reason to keep it
> mentioned.

libglade2 is also a direct dependency of etherape so it is fine to keep
it.



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