Bug 551651 - (ArpON) Review Request: ArpON - Arp handler inspection
Review Request: ArpON - Arp handler inspection
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Terje Røsten
Fedora Extras Quality Assurance
:
: 542991 (view as bug list)
Depends On:
Blocks: FE-SECLAB
  Show dependency treegraph
 
Reported: 2009-12-31 21:08 EST by Arun S A G
Modified: 2014-10-07 08:02 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-04-02 06:14:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
terjeros: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Arun S A G 2009-12-31 21:08:54 EST
Spec URL: http://sagarun.fedorapeople.org/SPECS/ArpON.spec
SRPM URL: http://sagarun.fedorapeople.org/SRPMS/ArpON-1.90-1.fc12.src.rpm

Description: 
ArpON (Arp handler inspectiON) is a portable handler daemon  It has a lot of features and it makes Arp a bit safer. It uses static arp inspection and dynamic arp inspection as two kinds of anti arp poisoning techniques.

koji:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1897321
Comment 1 Mohamed El Morabity 2010-01-01 06:49:27 EST
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
Comment 2 Arun S A G 2010-01-01 21:29:04 EST
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
Comment 3 Shakthi Kannan 2010-01-02 01:04:06 EST
#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
Comment 4 Mohamed El Morabity 2010-01-02 04:11:56 EST
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.
Comment 5 Mohamed El Morabity 2010-01-02 04:36:45 EST
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
Comment 6 Shakthi Kannan 2010-01-02 08:21:48 EST
(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.
Comment 7 Terje Røsten 2010-01-04 03:53:32 EST
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
Comment 8 Till Maas 2010-01-04 07:41:37 EST
There was an earlier review request submitted: bug #542991
please coordinate with the other submitter.
Comment 9 Arun S A G 2010-01-11 23:00:06 EST
*** Bug 542991 has been marked as a duplicate of this bug. ***
Comment 10 Arun S A G 2010-01-12 11:59:25 EST
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
Comment 11 Fabian Affolter 2010-03-03 03:51:16 EST
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*/*
Comment 12 Fabian Affolter 2010-03-03 03:55:14 EST
In the same effort the comment #5 can be honored.
Comment 13 Terje Røsten 2010-03-03 04:02:12 EST
> %{_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*
Comment 14 Till Maas 2010-03-03 04:18:35 EST
(In reply to comment #13)

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

Nothing, I prefer this, too.
Comment 15 Arun S A G 2010-03-06 14:25:01 EST
#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
Comment 16 Terje Røsten 2010-03-07 06:49:46 EST
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.
Comment 18 Terje Røsten 2010-03-09 11:35:25 EST
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.
Comment 19 Arun S A G 2010-03-09 12:23:34 EST
New Package CVS Request
=======================
Package Name: ArpON
Short Description: ARP handler inspection
Owners:sagarun
Branches: F-12,F-13
Comment 20 Jason Tibbitts 2010-03-10 17:17:15 EST
CVS done (by process-cvs-requests.py).
Comment 21 Terje Røsten 2010-03-21 16:47:35 EDT
Arun, what's up, forgot to import and build?
Comment 22 Arun S A G 2010-03-22 00:38:06 EDT
(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.
Comment 23 Fabian Affolter 2014-10-06 17:56:28 EDT
Package Change Request
======================
Package Name: ArpON
New Branches: epel7 el6
Owners: fab sagarun
InitialCC:
Comment 24 Gwyn Ciesla 2014-10-07 08:02:42 EDT
Git done (by process-git-requests).

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