Bug 463922 - (ifstat) Review Request: ifstat - Interface statistics
Review Request: ifstat - Interface statistics
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-25 09:46 EDT by Itamar Reis Peixoto
Modified: 2008-11-19 09:51 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-31 08:19:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Itamar Reis Peixoto 2008-09-25 09:46:06 EDT
Spec URL: http://ispbrasil.com.br/ifstat/ifstat.spec
SRPM URL: http://ispbrasil.com.br/ifstat/ifstat-1.1-1.2.fc9.src.rpm

Description:

ifstat(1) is a little tool to report interface activity like vmstat/iostat do.
In addition, ifstat can poll remote hosts through SNMP if you have the ucd-snmp
library. It will also be used for localhost if no other known method works (You
need to have snmpd running for this though).


ifstat seems to be cool, and I like it in fedora.
Comment 2 manuel wolfshant 2008-09-28 18:42:55 EDT
Could you please replace the "makeinstall" call with make install (plus proper arguments) ? The .spec file bundled in the tarball can give you some very good hints on how to do it properly. 
See https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used for reasoning behind my request.
Comment 3 Itamar Reis Peixoto 2008-09-29 17:46:25 EDT
please l@@k again

I have added a destdir patch, so I can use "make install"

I have also disabled debug info package, do you agree about this ?

rpmlint is showing a warning about file-not-utf8, can you help about this ?
ifstat.x86_64: W: file-not-utf8 /usr/share/doc/ifstat-1.1/README

here are the spec and the src.rpm 

http://ispbrasil.com.br/ifstat/ifstat.spec
http://ispbrasil.com.br/ifstat/ifstat-1.1-1.4.fc9.src.rpm
Comment 4 manuel wolfshant 2008-09-29 18:01:12 EDT
I am not sure that the destdir patch is needed, I'll try to check tomorrow.

And no, you should NOT disable building the debuginfo package. Exactly the opposite, you should make sure it contains useful info (i.e. the debug symbols stripped from the binaries; rpmbuild will do it for you automatically if you create a correct spec and you take care to not strip the generated binaries.)

As of the file-not-utf8 one, that is easy. Just use iconv to convert it. Something similar to
 iconv README -f ISO8859-1 -o README.utf8
 touch -r README  README.utf8
 mv README.utf8 README
will convert the file and preserve the timestamp.
BTW, you should use 
%{__make} install DESTDIR=$RPM_BUILD_ROOT CFLAGS="$RPM_OPT_FLAGS" INSTALL="install -p" 
in order to preserve the timestamps of the files. Or , alternatively, you could add the "-p" parameter to the install commands from your patch
Comment 5 Itamar Reis Peixoto 2008-09-30 00:12:41 EDT
the destdir patch is needed because the default makefile is buggy, after you look you will see. (without patch the makefile install ifstat in $(bindir) instead of $(DESTDIR)$(bindir))

about the debuginfo the package produces a empty debuginfo file, I have disabled because this

rpmlint /usr/src/redhat/RPMS/x86_64/ifstat-debuginfo-1.1-1.4.fc9.x86_64.rpm
ifstat-debuginfo.x86_64: E: empty-debuginfo-package
1 packages and 0 specfiles checked; 1 errors, 0 warnings.


about file-not-utf8, I have added a small patch to fix this instead using iconv.

here are new spec file and src.rpm


http://ispbrasil.com.br/ifstat/ifstat.spec
http://ispbrasil.com.br/ifstat/ifstat-1.1-1.5.fc9.src.rpm
Comment 6 Mamoru TASAKA 2008-09-30 01:08:40 EDT
I have no intention to review this request, however:

(In reply to comment #5)
> about the debuginfo the package produces a empty debuginfo file, I have
> disabled 

As wolfy said in the comment 4, this is not allowed. Instead you _must_
create a useful debuginfo rpm correctly.
The culprit which prevents from creating correct debuginfo rpm is in Makefile.in:
----------------------------------------------------------
    67  install-$(TARGET): $(TARGET)
    68          $(INSTALL) -d -m 755 $(DESTDIR)$(bindir)
    69          $(INSTALL) -p -s -m 755 $(TARGET) $(DESTDIR)$(bindir)/$(TARGET) <--------------
    70          $(INSTALL) -d -m 755 $(DESTDIR)$(mandir)/man1
    71          $(INSTALL) -p -m 644 ifstat.1 $(DESTDIR)$(mandir)/man1/ifstat.1
----------------------------------------------------------
On the line 69, "install" command uses "-s" option, which strips installed
binary, which needs fixing.

By the way build log shows:
----------------------------------------------------------
+ /usr/bin/make -j2
gcc -I. -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -O3 -fomit-frame-pointer -g0 -DHAVE_CONFIG_H  -DNETSNMP_ENABLE_IPV6 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -Ulinux -Dlinux=linux -I/usr/include/rpm -D_REENTRANT -D_GNU_SOURCE -DDEBUGGING -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I/usr/include/gdbm -I/usr/lib/perl5/5.10.0/i386-linux-thread-multi/CORE -I/usr/include/net-snmp -c ifstat.c -o ifstat.o
----------------------------------------------------------
Compilar flags "-O3" "-fomit-frame-pointer" makes debugging almost impossible so these flags
should be removed (note. the flag "-O3" is replaced by the latter "-O2", however it is better
that -O3 is removed anyway)
Comment 7 manuel wolfshant 2008-09-30 04:48:00 EDT
Please be as kind as to NOT alter the author's name via a so called patch. Use the proper method, i.e. iconv.
Comment 8 Itamar Reis Peixoto 2008-09-30 08:56:20 EDT
(In reply to comment #7)
> Please be as kind as to NOT alter the author's name via a so called patch. Use
> the proper method, i.e. iconv.

I have no intention to change the author name, but using iconv make it's unreadable, also using iconv makes spec files bigger, I have only converted the name of author to UTF8 removing "¨" [1] 

take a look

http://gael.roualland.free.fr/ifstat/HISTORY

I have changed from 

Gaël Roualland <gael.roualland@dial.oleane.com>

to

Gael Roualland <gael.roualland@dial.oleane.com>

the name still the same, but writen in UTF8


1 - http://en.wikipedia.org/wiki/Umlaut_(diacritic)
Comment 9 Itamar Reis Peixoto 2008-09-30 09:06:31 EDT
(In reply to comment #6)

>     69          $(INSTALL) -p -s -m 755 $(TARGET) $(DESTDIR)$(bindir)/$(TARGET)
> <--------------


BIG thanks to everyone, I am learning alot of things, I have removed -s , removed   "--enable-optim", from configure option, and added  --enable-debug, now I belive the debug_info package is being created.
Comment 11 Mamoru TASAKA 2008-10-02 04:22:34 EDT
(Just commenting again that as I guess I have no time to review this ticket
 for now, I appreciate if someone else would review this ticket)
Comment 12 Michael Schwendt 2008-10-16 19:04:07 EDT
* I don't see anything wrong with the suggested iconv line.

Hint: Run iconv on all files that need to be converted, then
create a diff to replace your patch.

If you disagree with the reviewer, don't jump to another
solution, but find out whether you really tried the same fix.


* --enable-debug in general is the wrong thing to do, especially if
it sets more than just compiler flags like -g. This program also sets
-DDEBUG which bears the risk of enabling and compiling optional code
that is not wanted. This particular program doesn't evaluate the DEBUG
define yet in its own source files, but other programs use such flags
to toggle assertions/debugging output and diagnostic code paths.

What you want instead is to simply ensure that the Fedora CFLAGS are
accepted by the Makefile and are not modified in unwanted ways.
Read the build log.


* Re: comment 4

> %{__make} install DESTDIR=$RPM_BUILD_ROOT CFLAGS="$RPM_OPT_FLAGS"
> INSTALL="install -p" 

Correct except for the CFLAGS definition, which must NOT be present
here. Nothing during "make install" is supposed to compile anything.

Also note that the %makeinstall macro may be used where a patch
like ifstat-destdir.patch were far bigger. That macro is a little
bit dangerous in some cases, so prefer the recommended make install
invocation.


* Licence is "GPLv2+" not "GPLv2". See header of the .c/.h files.
Note the "or [...] any later version".
Comment 13 Mamoru TASAKA 2008-10-24 12:52:36 EDT
Reis, would you update your srpm?
Comment 15 Mamoru TASAKA 2008-10-28 14:00:08 EDT
Seems good except for:

* Timestamp on original tarball is not kept
  https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

  Please re-download the original tarball by:
  $ wget -N http://gael.roualland.free.fr/ifstat/ifstat-1.1.tar.gz
  for example.

* Files under %_mandir are automatically marked as %doc.

* Please change the release number to X%{?dist} (2%{?dist}, for example)
  (I guess you want to do so after the review is passed)

Well,
+ This package itself is now okay (but please fix above)
+ You have also other review requests, which seems good to some
  extent

Okay.
--------------------------------------------------------------
    This package (itstat) is APPROVED by mtasaka
--------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji) ".

Now I am sponsoring you.

If you want to import this package into Fedora 8/9/10, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.
Comment 17 Mamoru TASAKA 2008-10-29 03:31:11 EDT
Okay, then please follow join wiki.
Comment 18 Itamar Reis Peixoto 2008-10-29 13:32:16 EDT
http://koji.fedoraproject.org/koji/taskinfo?taskID=910891

now I need to ask for a cvs account ?
Comment 19 Mamoru TASAKA 2008-10-29 13:52:38 EDT
Now you should follow Join wiki from "Add Package to CVS and Set Owner".
(as mentioned at http://fedoraproject.org/wiki/PackageMaintainers/Join#Add_Package_to_CVS_and_Set_Owner
 follow
 http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure )
Comment 20 Itamar Reis Peixoto 2008-10-29 16:57:27 EDT
New Package CVS Request
=======================
Package Name: ifstat
Short Description: Interface statistics
Owners: itamarjp
Branches: F-9 F-10
InitialCC: mtasaka
Comment 21 Kevin Fenzi 2008-10-29 17:26:27 EDT
cvs done.
Comment 22 Fedora Update System 2008-10-30 13:48:42 EDT
ifstat-1.1-8.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/ifstat-1.1-8.fc9
Comment 23 Fedora Update System 2008-10-31 06:23:36 EDT
ifstat-1.1-8.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update ifstat'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-9312
Comment 24 Fedora Update System 2008-11-19 09:51:08 EST
ifstat-1.1-8.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

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