Bug 551651 (ArpON)

Hi,

here is an informal review of your package, while waiting to be sponsored ^^.
Just a few remarks, the general look of the .spec seems pretty good ^^.

Be very careful, your binary is not build using the Fedora Project flags currently: see this piece of build log that shows it:
+ /usr/bin/make linux
gcc -g -g -Wall -Werror  -lpthread -lpcap -ldnet -lnet -L/usr/local/lib -I/usr/local/include -DLINUX -o arpon arpon.c
+ exit 0

You must specify these flags in your make command, like this:
   %build
   %{__make} CFLAGS="$RPM_OPT_FLAGS" linux 

By the way, you should leave in make the parallel build option by default:
   %build
   %{__make} %{?_smp_mflags} CFLAGS="$RPM_OPT_FLAGS" linux 
No particular reason to skip it in my opinion, even if there is only one file to compile... For the moment ^^.

You should also remove the INSTALL file, it is useless if delivered with a package ^^
   See https://fedoraproject.org/wiki/Packaging/Guidelines#Documentation

Fixed the issues mentioned above:

Spec URL: http://sagarun.fedorapeople.org/SPECS/ArpON.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/ArpON-1.90-2.fc12.src.rpm

#001 The Makefile links -lpthread, and so BuildRequires needs to include glibc-devel.

  BuildRequires: glibc-devel

#002 The Makefile is using -L/usr/local/lib and -I/usr/local/include. But, your shipped package will be in /usr. So, you need to replace these references in the Makefile to use %{_libdir} and %{_includedir} respectively in %setup section.

%{__sed} -e "s|/usr/local/lib|%{_libdir}|" Makefile > Makefile.ex
touch -r Makefile Makefile.ex
%{__mv} Makefile.ex Makefile

%{__sed} -e "s|/usr/local/include|%{_includedir}|" Makefile > Makefile.ex
touch -r Makefile Makefile.ex
%{__mv} Makefile.ex Makefile

Shakthi : about your comment #001: since glibc-devel is a dependancy of gcc, and since gcc is part of the minimal build system in Fedora (see https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2), there is no need to set glibc-devel as a BR. By the way the RPM builds fine in mock without it.

About #002: moreover, I don't think there is a real need to modify the Makefile. /usr/lib and /usr/include (or /usr/lib64 and /usr/include when running Fedora on a x86_64) already belong to the search paths by default of gcc. It's not a problem to leave the Makefile looking for headers and libs in /usr/local since this directory is empty anyway in a mock chroot.

Arun: just a cosmetic issue about the $RPM_OPT_FLAGS variable I suggested before. In fact, you should replace $RPM_OPT_FLAGS by %{optflags}, since you are using a "macro-style" in your .spec

(In reply to comment #4)
| About #002: moreover, I don't think there is a real need to modify the
| Makefile. /usr/lib and /usr/include (or /usr/lib64 and /usr/include when
| running Fedora on a x86_64) already belong to the search paths by default of
| gcc. It's not a problem to leave the Makefile looking for headers and libs in
| /usr/local since this directory is empty anyway in a mock chroot.  
\--

It is not just a question of it working in mock chroot, but, how upstream sources should use it on different platforms, and target builds.

Ideally, upstream should use a PREFIX for the install variable, and not hard-code it everywhere in the Makefile. From the .spec file one should be able to override the PREFIX with the preferred location.

Thanks for your feedback.

Your source url is wrong, use

http://downloads.sourceforge.net, not a specific mirror and %{version} macro:
 
http://downloads.sourceforge.net/project/arpon/arpon/ArpON-%{version}/ArpON-%{version}.tar.gz

Download with wget -N to get correct timestamp on tarball.

Ref: https://fedoraproject.org/wiki/Packaging:SourceURL

%{__install} -pm 755 -d %{buildroot}%{_sbindir}
%{__install} -pm 755 -d %{buildroot}%{_mandir}/man8/
%{__install} -pm 755 arpon %{buildroot}%{_sbindir}
%{__install} -pm 644 man8/arpon.8 %{buildroot}%{_mandir}/man8/

Could be done as

%{__install} -D -pm 755 arpon %{buildroot}%{_sbindir}/arpon
%{__install} -D -pm 644 man8/arpon.8 %{buildroot}%{_mandir}/man8/arpon.8

There was an earlier review request submitted: bug #542991
please coordinate with the other submitter.

*** Bug 542991 has been marked as a duplicate of this bug. ***

Fixed issues reported by  Terje Røsten:

SPEC URL: http://sagarun.fedorapeople.org/SPECS/ArpON.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/ArpON-1.90-3.fc12.src.rpm

Just another comment on your spec file

%{_mandir}/man8/arpon.8.gz

Since manual pages are compressed on-the-fly by rpmbuild and the compression
tool may change, it's prefered to use:  %{_mandir}/man*/*

In the same effort the comment #5 can be honored.

> %{_mandir}/man8/arpon.8.gz
> 
> Since manual pages are compressed on-the-fly by rpmbuild and the compression
> tool may change, it's prefered to use:  %{_mandir}/man*/*    

This seems too generic? What's wrong with: %{_mandir}/man8/arpon.8*

(In reply to comment #13)

> This seems too generic? What's wrong with: %{_mandir}/man8/arpon.8*    

Nothing, I prefer this, too.

#Comment 5 honored 
# Fixed mandir to %{_mandir}/man8/arpon.8*

SPEC URL: http://sagarun.fedorapeople.org/SPECS/ArpON.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/ArpON-1.90-4.fc12.src.rpm

Cut and paste error?

%{__install} -D -pm 644 man8/arpon.8 %{buildroot}%{_mandir}/man8/arpon.8*


Some pedantic things in changelog
 - don't mix case on first letter
 - don't mix ending with/without .

Please use only one empty between sections, and I don't see the need for
empty lines in top tags at all.

Rest seems fine, fix these and I will do the formal review.

Fixed issues in #comment 16

SPEC URL: http://sagarun.fedorapeople.org/SPECS/ArpON.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/ArpON-1.90-5.fc12.src.rpm

o rpmlint clean
o spec file looks fine
o naming is fine
o ownership/perms is correct
o sha is good:
$ sha1sum ArpON-1.90.tar.gz*
   0a0dbfa6e7571892b39bcd25b91f14c840b01315  ArpON-1.90.tar.gz
   0a0dbfa6e7571892b39bcd25b91f14c840b01315  ArpON-1.90.tar.gz.spec
! please download with e.g. wget -N to get correct time stamp on tarball.
o license is BSD and set correct.
o builds with correct flags:
   http://koji.fedoraproject.org/koji/getfile?taskID=2041933&name=build.log
o builds in koji:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=2041927
o man page included

-> Fix the time stamp when importing.


  The package ArpON is APPROVED.

New Package CVS Request
=======================
Package Name: ArpON
Short Description: ARP handler inspection
Owners:sagarun
Branches: F-12,F-13

CVS done (by process-cvs-requests.py).

Arun, what's up, forgot to import and build?

(In reply to comment #21)
> Arun, what's up, forgot to import and build?    

Sorry, i was busy last weekend. Will import it this week.

Package Change Request
======================
Package Name: ArpON
New Branches: epel7 el6
Owners: fab sagarun
InitialCC:

Git done (by process-git-requests).