[Bug 551651] New: Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: ArpON - Arp handler inspection

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Summary: Review Request: ArpON - Arp handler inspection
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

ELMORABITY Mohamed  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |melmorabity@fedoraproject.o
                   |                            |rg

--- Comment #1 from ELMORABITY Mohamed   2010-01-01 06:49:27 EDT ---
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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #2 from Arun SAG   2010-01-01 21:29:04 EDT ---
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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Shakthi Kannan  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #3 from Shakthi Kannan   2010-01-02 01:04:06 EDT ---
#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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #4 from ELMORABITY Mohamed   2010-01-02 04:11:56 EDT ---
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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #5 from ELMORABITY Mohamed   2010-01-02 04:36:45 EDT ---
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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #6 from Shakthi Kannan   2010-01-02 08:21:48 EDT ---
(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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Terje Røsten  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #7 from Terje Røsten   2010-01-04 03:53:32 EDT ---
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-%%7Bversion%7D/Ar...

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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Till Maas  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #8 from Till Maas   2010-01-04 07:41:37 EDT ---
There was an earlier review request submitted: bug #542991
please coordinate with the other submitter.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Fabian Affolter  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
             Blocks|                            |563471(FE-SECSPIN)

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #11 from 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*/*

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #12 from Fabian Affolter  2010-03-03 03:55:14 EST ---
In the same effort the comment #5 can be honored.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #13 from 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*

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #14 from 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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #15 from Arun SAG  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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #16 from 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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #17 from Arun SAG  2010-03-09 09:47:17 EST ---
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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Terje Røsten  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Terje Røsten  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #18 from 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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Arun SAG  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #19 from Arun SAG  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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #20 from Jason Tibbitts  2010-03-10 17:17:15 EST ---
CVS done (by process-cvs-requests.py).

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Fabian Affolter  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Alias|                            |ArpON

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

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

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

--- Comment #22 from Arun SAG  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.

[Bug 551651] Review Request: ArpON - Arp handler inspection

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=551651

Arun SAG  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |ERRATA