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 551651 (ArpON) - Review Request: ArpON - Arp handler inspection
Summary: Review Request: ArpON - Arp handler inspection
Keywords:
Status: CLOSED ERRATA
Alias: ArpON
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Terje Røsten
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 542991 (view as bug list)
Depends On:
Blocks: FE-SECLAB
TreeView+ depends on / blocked
 
Reported: 2010-01-01 02:08 UTC by Arun S A G
Modified: 2014-10-07 12:02 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-04-02 10:14:31 UTC
Type: ---
Embargoed:
terje.rosten: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Arun S A G 2010-01-01 02:08:54 UTC
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 11:49:27 UTC
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-02 02:29:04 UTC
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 06:04:06 UTC
#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 09:11:56 UTC
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 09:36:45 UTC
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 13:21:48 UTC
(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 08:53:32 UTC
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 12:41:37 UTC
There was an earlier review request submitted: bug #542991
please coordinate with the other submitter.

Comment 9 Arun S A G 2010-01-12 04:00:06 UTC
*** Bug 542991 has been marked as a duplicate of this bug. ***

Comment 10 Arun S A G 2010-01-12 16:59:25 UTC
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 08:51:16 UTC
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 08:55:14 UTC
In the same effort the comment #5 can be honored.

Comment 13 Terje Røsten 2010-03-03 09:02:12 UTC
> %{_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 09:18:35 UTC
(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 19:25:01 UTC
#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 11:49:46 UTC
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 16:35:25 UTC
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 17:23:34 UTC
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 22:17:15 UTC
CVS done (by process-cvs-requests.py).

Comment 21 Terje Røsten 2010-03-21 20:47:35 UTC
Arun, what's up, forgot to import and build?

Comment 22 Arun S A G 2010-03-22 04:38:06 UTC
(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 21:56:28 UTC
Package Change Request
======================
Package Name: ArpON
New Branches: epel7 el6
Owners: fab sagarun
InitialCC:

Comment 24 Gwyn Ciesla 2014-10-07 12:02:42 UTC
Git done (by process-git-requests).


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