Bug 576591 - Review Request: iptraf-ng
Summary: Review Request: iptraf-ng
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Terje Røsten
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-03-24 14:54 UTC by Nikola Pajkovsky
Modified: 2014-02-02 22:14 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-02-07 16:54:23 UTC
Type: ---
Embargoed:
terje.rosten: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Nikola Pajkovsky 2010-03-24 14:54:32 UTC
spec: http://npajkovs.fedorapeople.org/iptraf-ng.spec
srpm: http://npajkovs.fedorapeople.org/iptraf-ng-1.0.2-1.fc14.src.rpm

This package is in conflict with iptraf. IPTraf has dead upstream so I've made fork of original.

Decrtipion:
IPTraf-ng is a console-based network monitoring utility.  IPTraf gathers
data like TCP connection packet and byte counts, interface statistics
and activity indicators, TCP/UDP traffic breakdowns, and LAN station
packet and byte counts.  IPTraf-ng features include an IP traffic monitor
which shows TCP flag information, packet and byte counts, ICMP
details, OSPF packet types, and oversized IP packet warnings;
interface statistics showing IP, TCP, UDP, ICMP, non-IP and other IP
packet counts, IP checksum errors, interface activity and packet size
counts; a TCP and UDP service monitor showing counts of incoming and
outgoing packets for common TCP and UDP application ports, a LAN
statistics module that discovers active hosts and displays statistics
about their activity; TCP, UDP and other protocol display filters so
you can view just the traffic you want; logging; support for Ethernet,
FDDI, ISDN, SLIP, PPP, and loopback interfaces; and utilization of the
built-in raw socket interface of the Linux kernel, so it can be used
on a wide variety of supported network cards.

Comment 1 Terje Røsten 2010-03-24 21:52:32 UTC
Okay, you has of course informed upstream IPTraf about the fork?

Comments on the spec:

  %configure --enable-shared=no --enable-static=yes

Why?
Fedora tend to do the reverse.

  rm -rf Documentation/.xvpics

If your are upstream maintainer this could be remove in the tarball?

  mkdir -p $RPM_BUILD_ROOT/etc/logrotate.d/
  cp %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf

Change to 
 install -D -m 0644 -p %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf

  %attr(755,root,root) %{_prefix}/bin/*

Remove %attr and, use bindir macro and list explicit.

  %{_mandir}/*/*

Not so general please.

  %dir %attr(644,root,root) %config(noreplace) /etc/logrotate.d/iptraf

Drop %attr, %dir seems wrong.

  - Initialization build

I leave that to a native speaker.

Comment 2 Nikola Pajkovsky 2010-04-05 15:09:39 UTC
(In reply to comment #1)
> Okay, you has of course informed upstream IPTraf about the fork?
> 
> Comments on the spec:
> 
>   %configure --enable-shared=no --enable-static=yes
>
> 
> Why?
> Fedora tend to do the reverse.

I know. I have it this issue in my TODO. Originaly iptraf is built with "support" library and it has never been shipped. It's used as helper to build gui in console and it is linked statically. This option say to autotools to not generate *.so files.

> 
>   rm -rf Documentation/.xvpics
> 
> If your are upstream maintainer this could be remove in the tarball?
> 

Yes it is done in git repo and it will be removed with next release.(I've made note for myself in spec)

>   mkdir -p $RPM_BUILD_ROOT/etc/logrotate.d/
>   cp %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf
> 
> Change to 
>  install -D -m 0644 -p %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf
> 
Fixed.

>   %attr(755,root,root) %{_prefix}/bin/*
> 
> Remove %attr and, use bindir macro and list explicit.
> 
>   %{_mandir}/*/*
> 
> Not so general please.
Fixed.

> 
>   %dir %attr(644,root,root) %config(noreplace) /etc/logrotate.d/iptraf
> 
> Drop %attr, %dir seems wrong.
> 
Fixed.

>   - Initialization build
> 
> I leave that to a native speaker.    
I don't get it.

New spec and srpm:
spec: http://npajkovs.fedorapeople.org/iptraf-ng.spec
srpm: http://npajkovs.fedorapeople.org/iptraf-ng-1.0.2-2.fc14.src.rpm

Comment 3 Mamoru TASAKA 2010-04-05 15:43:06 UTC
(fedora-review flag must not be set by the submitter but must be
 by the formal reviewer. Once unsetting)

Comment 4 Terje Røsten 2010-04-06 18:39:55 UTC
Thanks, good progress, some more comments.

Uou seem to be fan of %attr in %files, in general we only use that if
something is special or uncommon, in your case I see no such things.
Set bits correct in %files and let the %files section be simple.

I would prefer that you changed:

mkdir -p $RPM_BUILD_ROOT/etc/logrotate.d/
cp %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf

mkdir -p $RPM_BUILD_ROOT/var/lock/iptraf
mkdir -p $RPM_BUILD_ROOT/var/log/iptraf
mkdir -p $RPM_BUILD_ROOT/var/lib/iptraf

to:

install -D -p -m 0644 %{SOURCE1} $RPM_BUILD_ROOT%{_sysconfdir}/logrotate.d/iptraf-ng

# reduce to one line, preserve timestamps, correct perms. use proper macro and change to iptraf-ng (the proper name)

and 

install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/{lock,log,lib}/iptraf-ng

# same thing here.

Now with %install fixed, these lines in %files:

%attr(755,root,root) %{_prefix}/bin/*
%{_mandir}/*/*
%dir %attr(755,root,root) /var/lock/iptraf
%dir %attr(755,root,root) /var/log/iptraf
%dir %attr(755,root,root) /var/lib/iptraf
%dir %attr(644,root,root) %config(noreplace) /etc/logrotate.d/iptraf

should be

%{_bindir}/iptraf-ng
%{_bindir}/rawtime
%{_bindir}/rvnamed
%{_mandir}/man8/iptraf.8*
%{_mandir}/man8/rvnamed.8*
%{_localstatedir}/lock/iptraf-ng
%{_localstatedir}/log/iptraf-ng
%{_localstatedir}/lib/iptraf-ng
%config(noreplace) %{_sysconfdir}/logrotate.d/iptraf-ng


BTW: the rawtime name is a bit on the generic side, a man page about
the tool would not hurt.
 
You should really decide on iptraf versus iptraf-ng.

Is this iptraf og iptraf-ng?

Comment 5 Nikola Pajkovsky 2010-04-08 08:48:31 UTC
(In reply to comment #4)
> Thanks, good progress, some more comments.
> 
> Uou seem to be fan of %attr in %files, in general we only use that if
:D I'm definitely not a fan of writing spec files and mainly rewriting spec.

> something is special or uncommon, in your case I see no such things.
> Set bits correct in %files and let the %files section be simple.
> 
> I would prefer that you changed:
> 
> mkdir -p $RPM_BUILD_ROOT/etc/logrotate.d/
> cp %{SOURCE1} $RPM_BUILD_ROOT/etc/logrotate.d/iptraf
> 
> mkdir -p $RPM_BUILD_ROOT/var/lock/iptraf
> mkdir -p $RPM_BUILD_ROOT/var/log/iptraf
> mkdir -p $RPM_BUILD_ROOT/var/lib/iptraf
> 
> to:
> 
> install -D -p -m 0644 %{SOURCE1}
> $RPM_BUILD_ROOT%{_sysconfdir}/logrotate.d/iptraf-ng
> 
Fixed.

> # reduce to one line, preserve timestamps, correct perms. use proper macro and
> change to iptraf-ng (the proper name)
> 
> and 
> 
> install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/{lock,log,lib}/iptraf-ng
> 
Fixed. I didn't know I can you {brum,brum}
> # same thing here.
> 
> Now with %install fixed, these lines in %files:
> 
> %attr(755,root,root) %{_prefix}/bin/*
> %{_mandir}/*/*
> %dir %attr(755,root,root) /var/lock/iptraf
> %dir %attr(755,root,root) /var/log/iptraf
> %dir %attr(755,root,root) /var/lib/iptraf
> %dir %attr(644,root,root) %config(noreplace) /etc/logrotate.d/iptraf
> 
> should be
> 
> %{_bindir}/iptraf-ng
> %{_bindir}/rawtime
> %{_bindir}/rvnamed
> %{_mandir}/man8/iptraf.8*
> %{_mandir}/man8/rvnamed.8*
> %{_localstatedir}/lock/iptraf-ng
> %{_localstatedir}/log/iptraf-ng
> %{_localstatedir}/lib/iptraf-ng
> %config(noreplace) %{_sysconfdir}/logrotate.d/iptraf-ng
> 
Fixed.

> 
> BTW: the rawtime name is a bit on the generic side, a man page about
> the tool would not hurt.
> 
> You should really decide on iptraf versus iptraf-ng.
> 
> Is this iptraf og iptraf-ng?    

This is iptraf-ng and I left the original naming. There is some *wild* header called dirs.h. It works as config.h in autotools and in my TODO is get rid of this machinery and use config.h. I'm mostly working on ABRT so I don't have much time, but I feel in my bones that this weekend this should be done :)

I hope this is a last round :)

spec: http://npajkovs.fedorapeople.org/iptraf-ng.spec
srpm: http://npajkovs.fedorapeople.org/iptraf-ng-1.0.2-3.fc14.src.rpm

Comment 6 Chen Lei 2010-06-24 17:19:07 UTC
If the original iptraf upstream is dead, then you can use Obsoletes/Provides instead of Conflicts

See packaging guideline

Comment 7 Jason Tibbitts 2010-11-23 04:20:49 UTC
Doesn't look like anything has happened with this ticket in quite some time.

A few random comments:

The previous comment is correct; it is not appropriate to conflict with iptraf.  Instead, you should have a proper Obsoletes:/Provides: pair.  The guideline that was being referred to is http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Renaming.2Freplacing_existing_packages

The original iptraf spec looks a bit cleaner than this one; for some reason those terrible macros like "%{__rm}" snuck in.  If you for some reason like the extra typing and decreased readability and really want to use them, you need to be consistent about it and use them everywhere (which means using "%{__install}" and not using plain "rm").  Or you could just drop them entirely.  (Honestly I dislike them enough that I simply don't review packages that use them.)

I see that development is continuing; what's the current version?

Comment 8 Nikola Pajkovsky 2010-11-23 09:46:39 UTC
I'm working on huge clean up...when I release a new version, I will put here a new version for an inspection.

Comment 9 Nikola Pajkovsky 2011-01-25 13:12:42 UTC
http://npajkovs.fedorapeople.org/iptraf-ng.spec
http://npajkovs.fedorapeople.org/iptraf-ng-1.0.3.52.gdaa1-1.fc15.src.rpm

got rid of %{__boohoo}, proper Obsoletes/Provides pair. There is a lot of more under hood.

Comment 10 Terje Røsten 2011-01-26 09:53:34 UTC
Thanks, this is starting to look good. 

Some warnings from rpmlint:

W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)

- convert tabs in Obsoletes: and Provides: to spaces.

W: unversioned-explicit-obsoletes iptraf
W self-obsoletion iptraf obsoletes iptraf = 1.0.3.52.gdaa1-1.fc14

- these seems dangerous, 

W: invalid-url Source0: https://fedorahosted.org/releases/i/p/iptraf-ng/iptraf-ng-1.0.3.52.gdaa1.tar.gz HTTP Error 404: Not Found

- can't find the tarball on the website, 
 would also be nice to remove the gdaa1 postfix.

W: incoherent-version-in-changelog 1.0.2-3 ['1.0.3.52.gdaa1-1.fc14', '1.0.3.52.gdaa1-1']

- easy to fix

W: manual-page-warning /usr/share/man/man8/iptraf-ng.8.gz 27: warning: macro `Biptraf' not defined


Fix these and I will do the formal review.

Comment 11 Nikola Pajkovsky 2011-01-26 10:30:13 UTC
http://npajkovs.fedorapeople.org/iptraf-ng.spec
http://npajkovs.fedorapeople.org/iptraf-ng-1.0.3.55.gae6e.dirty-1.fc15.src.rpm

(In reply to comment #10)
> Thanks, this is starting to look good. 
> 
> Some warnings from rpmlint:
> 
> W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)
> 
> - convert tabs in Obsoletes: and Provides: to spaces.
> 
> W: unversioned-explicit-obsoletes iptraf
> W self-obsoletion iptraf obsoletes iptraf = 1.0.3.52.gdaa1-1.fc14
> 
> - these seems dangerous, 
> 

fixed

> W: invalid-url Source0:
> https://fedorahosted.org/releases/i/p/iptraf-ng/iptraf-ng-1.0.3.52.gdaa1.tar.gz
> HTTP Error 404: Not Found
> 
> - can't find the tarball on the website, 
>  would also be nice to remove the gdaa1 postfix.

the naming convention is A.B.C.X.sha1.dirty-%{release}

where,
 
    A.B.C - is version of iptraf-ng
    X  - is how many commits are iptraf ahead of A.B.C version
    sha1 - taken from git description
    optional dirty - show only if I have dirty working directory (Right now I middle of some work)

This is a snapshot taken from git. For table release it will be looking as iptraf-ng.A.B.C-%{release}

> W: incoherent-version-in-changelog 1.0.2-3 ['1.0.3.52.gdaa1-1.fc14',
> '1.0.3.52.gdaa1-1']
> 
> - easy to fix
> 
> W: manual-page-warning /usr/share/man/man8/iptraf-ng.8.gz 27: warning: macro
> `Biptraf' not defined
> 
> 
> Fix these and I will do the formal review.

This won't be fixed till I release a new stable version or I put iptraf-ng into rawhide. 

Thank you for taking time to look at it.

Comment 12 Terje Røsten 2011-01-26 11:04:21 UTC
Version:        1.0.3.55.gae6e.dirty

[snip]

Obsoletes:      iptraf <= 3.0.1-10
Provides:       iptraf = %{version}-%{release}


This will not work, with %{version} < 3.0.1-10 you will obsolete your self.
Your need to provide iptraf > 3.0.1-10.

Comment 13 Nikola Pajkovsky 2011-01-26 12:36:07 UTC
I'm a little puzzled here. How can I obsolete by my self if the package name is iptraf-ng not iptraf?

or should it be like that?

Version:        1.0.3.55.gae6e.dirty

[snip]

Obsoletes:      iptraf <= 3.0.1-10
Provides:       iptraf-ng = %{version}

Comment 14 Terje Røsten 2011-01-26 13:01:52 UTC
No, don't help.

This line

Provides:       iptraf-ng = %{version}

is implicit any way. 

This line

Obsoletes:      iptraf <= 3.0.1-10

is correct.

This means when iptraf-ng is installed iptraf is removed, I guess you want that to happen

Now if some installed package A have a dep on iptraf,
rpm/yum will refuse to install iptraf-ng as iptraf is removed with nothing to satisfy the dep package A has on iptraf. So we solve that by adding

Provides:       iptraf = %{version}-%{release}

in iptraf-ng.spec, letting iptraf-ng satisfy the dep pacakge A has on iptraf.

However, we have just obsoleted iptraf <= 3.0.1-10, when
%{version}-%{release} < 3.0.1-10 the package obsoletes itself, not good.

I see two options:

- adjust %{version} to be > 3.0.1

or use this line

Provides:       iptraf = 3.0.1-11

Comment 15 Nikola Pajkovsky 2011-01-26 13:39:49 UTC
Thank you, second solution doesn't work :-/ I've put this into spec file

Obsoletes:      iptraf < 3.1
Provides:       iptraf = 3.1

Comment 16 Nikola Pajkovsky 2011-01-27 12:50:11 UTC
what do you think? Is it ok with you?

Comment 17 Terje Røsten 2011-01-27 13:50:35 UTC
Yes, seems good, thanks.

Will try to find free time to do the formal review soon.

Comment 18 Terje Røsten 2011-01-28 18:39:42 UTC
Formal review:

ok - package meets naming and versioning guidelines
! - source files match upstream:
   no tarball available
ok - specfile is properly named, is cleanly written and uses macros consistently
ok - dist tag is present
ok - build root is correct
! - license field matches the actual license
  most parts is GPLv2+, however some files has unclear license:
  bar.c
  cidr.c
  getpath.c
  ipcsum.c
  mode.c
  tr.c
 most *.h  files are missing license info.
   You must contact the author (Gerard Paul Java) about these problems.
ok - license is open source-compatible
ok - license text included in package
ok - latest version is being packaged
ok - BuildRequires are proper and compiler flags are appropriate
 koji is happy, however there are some warning you might want to look at:
 http://koji.fedoraproject.org/koji/getfile?taskID=2748127&name=build.log
ok - %clean is present
ok - package builds in koji
  http://koji.fedoraproject.org/koji/taskinfo?taskID=2748127
ok - package installs properly, even works
ok - debuginfo package looks complete
! rpmlint is silent
 invalid-url Source0: https://fedorahosted.org/releases/i/p/iptraf-ng/iptraf-ng-1.0.3.55.gae6e.dirty.tar.gz HTTP Error 404: Not Found
 incoherent-version-in-changelog 1.0.2-3 ['1.0.3.55.gae6e.dirty-1.fc13', '1.0.3.55.gae6e.dirty-1']
ok - final provides and requires are sane
ok - owns the directories it creates
ok -doesn't own any directories it shouldn't
ok -no duplicates in %files
ok -file permissions are appropriate
ok- documentation is small, so no -docs subpackage is necessary
ok -%docs are not necessary for the proper functioning of the package

Summary:

 o include the obsolete/provides in comment #15.
 o upload a tarball
 o fix changelog
 o fix the license issue

Comment 19 Terje Røsten 2011-02-28 12:46:00 UTC
ping?

Comment 20 Nikola Pajkovsky 2011-02-28 12:53:28 UTC
yes?

Comment 21 Terje Røsten 2011-02-28 12:57:03 UTC
I just wonder about any progress with the issues in comment #18.

You still want the package included in Fedora?

Comment 22 Nikola Pajkovsky 2011-02-28 13:40:55 UTC
Sure I want iptraf-ng in Fedora ;)

o include the obsolete/provides in comment #15.
 - fixed in commit 7abf0de222f3a

o upload a tarball
o fix changelog
 - I will fix it when git will be ready and iptraf-ng-1.1.0 ready to deploy.

o fix the license issue
  bar.c - just print "Computing" on status bar. I have a plan to create an universal function for bar operation. So this module will be removed. 
  cidr.c
  getpath.c - module will be removed and I will provide an option in configure
  ipcsum.c - if the ping is under GPLv2+ than even this module must be, because in_cksum() was taken from ping.
  mode.c - omg!!! I will remove it completely and provide only one simple macro
  tr.c - tricky one. The code was taken from Linux kernel in 2002.

"You must contact the author (Gerard Paul Java) about these problems."

I'm not sure if this will be doable. Upstream is dead and he didn't response. I will try it again.

Comment 23 Terje Røsten 2011-02-28 14:20:57 UTC
Thanks for the feedback, please update the ticket when you have 1.1.0 ready.

Comment 24 Terje Røsten 2011-10-17 13:04:41 UTC
Hi again, what is current status?

Comment 25 Nikola Pajkovsky 2011-12-20 15:36:26 UTC
new status of iptraf-ng

  ipcsum.c  - rewritten
  mode.c    - removed
  bar.c     - licence changed
  cidr.c    - licence changed
  getpath.c - licence changed
  tr.c      - licence changed

build is in koji
http://koji.fedoraproject.org/koji/taskinfo?taskID=3595694

http://npajkovs.fedorapeople.org/iptraf-ng.spec
http://npajkovs.fedorapeople.org/iptraf-ng-1.1.0.rc0-1.el6.src.rpm

Comment 26 Terje Røsten 2011-12-20 20:25:02 UTC
We are soon there, some small things from rpmlint:

W: incoherent-version-in-changelog 1.1.0-rc0-1 ['1.1.0.rc0-1.fc14', '1.1.0.rc0-1']
E: incorrect-fsf-address /usr/share/doc/iptraf-ng-1.1.0.rc0/LICENSE
W: install-file-in-docs /usr/share/doc/iptraf-ng-1.1.0.rc0/INSTALL
W: non-ghost-in-var-lock /var/lock/iptraf-ng

Fix these and we should be all good. At least fix the error about address.

Comment 27 Nikola Pajkovsky 2012-01-05 09:04:00 UTC
W: non-ghost-in-var-lock /var/lock/iptraf-ng

I don't have any idea, how did you get above message.

All, except incoherent-version-in-changelog, warnings and error are fixed. I'm auto generating spec version of iptraf-ng from current git version, and will be off almost every time, except the time of tagging of new version in git.

http://npajkovs.fedorapeople.org/iptraf-ng-1.1.0.rc0.1.gfe0c-2.el6.src.rpm

please pick spec file from srpm

Comment 28 Terje Røsten 2012-01-05 19:56:30 UTC
Thanks,

 package iptrag-ng is APPROVED.

Comment 29 Nikola Pajkovsky 2012-01-06 02:29:26 UTC
juuuhuuu! many thanks for review

please create someone git repo for f15 till the rawhide

Comment 30 Terje Røsten 2012-01-06 15:42:47 UTC
Please create a proper SCM request:

 http://fedoraproject.org/wiki/Package_SCM_admin_requests

Comment 31 Nikola Pajkovsky 2012-01-10 12:39:09 UTC
New Package SCM Request
=======================
Package Name: iptraf-ng
Short Description: A console-based network monitoring utility
Owners: npajkovs
Branches: f15 f16
InitialCC:

Comment 32 Gwyn Ciesla 2012-01-10 13:01:01 UTC
Git done (by process-git-requests).

Comment 33 Nikola Pajkovsky 2012-01-24 14:34:47 UTC
New Package SCM Request
=======================
Package Name: iptraf-ng
Short Description: A console-based network monitoring utility
Owners: npajkovs
Branches: el5 el6
InitialCC:

Comment 34 Terje Røsten 2012-02-01 22:07:13 UTC
Please close ticket if package is imported.

Comment 35 Juha Tuomala 2012-02-02 10:07:57 UTC
EPEL builds would be highly appreciated.

Comment 36 Nikola Pajkovsky 2012-02-02 10:25:38 UTC
I'm going to create another ticket for rel-eng

Comment 37 Nikola Pajkovsky 2012-02-07 06:49:16 UTC
New Package SCM Request
=======================
Package Name: iptraf-ng
Short Description: A console-based network monitoring utility
Owners: npajkovs
Branches: el5 el6
InitialCC:

Comment 38 Gwyn Ciesla 2012-02-07 13:35:15 UTC
Already exists, to create new branches submit a Package Change request.

http://fedoraproject.org/wiki/Package_SCM_admin_requests

Thanks!

Comment 39 Nikola Pajkovsky 2012-02-07 16:54:23 UTC
Package Change Request
======================
Package Name: iptraf-ng
New Branches: el5 el6
Owners: npajkovs

Comment 40 Gwyn Ciesla 2012-02-08 13:10:37 UTC
Git done (by process-git-requests).


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