Bug 460887

Summary: Review Request: libpcapnav - a libpcap trace file navigation library
Product: [Fedora] Fedora Reporter: Christian <christian>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: andreas, fedora-package-review, gwync, itamar, mtasaka, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-11-25 03:17:47 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449, 460886    

Description Christian 2008-09-02 11:59:40 UTC
Spec URL: http://www.icir.org/christian/fedora/libpcapnav.spec
SRPM URL: http://www.icir.org/christian/fedora/libpcapnav-0.8-1.src.rpm
Description: libpcapnav is a libpcap wrapper library that allows navigation to
arbitrary locations in a tcpdump trace file between reads. The API is
intentionally much like that of the pcap library. You can navigate in
trace files both in time and space: you can jump to a packet which is
at approximately 2/3 of the trace, or you can jump as closely as
possible to a packet with a given timestamp, and then read packets
from there.

Please note: I need a sponsor.

Comment 1 Mamoru TASAKA 2008-09-12 05:50:41 UTC
Your srpm does not build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=822047

Comment 2 Michael Schwendt 2008-10-11 10:52:41 UTC
> %define prefix   /usr

Please give the rationale for making this package relocatable:
https://fedoraproject.org/wiki/Packaging/ReviewGuidelines

Note that in your case it is a mistake:
pcapnav-config isn't relocatable.


> $ rpmlint libpcapnav-0.8-1.src.rpm 
> libpcapnav.src:18: W: hardcoded-packager-tag Christian

Don't set "Packager". Your name appears in the %changelog.
Setting the Packager tag should only be done in your local
build-system for binary rpms *you* have built. Publishing
src.rpms with a default Packager tag bears the risk that
other people build and publish bad binary rpms with your
name in the package header.

> libpcapnav.src:20: W: hardcoded-prefix-tag %{prefix}

See top of review.

> libpcapnav.src:47: W: configure-without-libdir-spec

Prefer the %configure macro where it works.
Run "rpm --eval %configure" to see what it does.
It also defines libdir as necessary.

> libpcapnav.src:82: W: macro-in-%changelog prefix

Avoid macros in %changelog. Some cause damage when they expand.
You can escape them with a double %% as in %%prefix.

> libpcapnav.src: W: no-version-in-last-changelog

It's preferred if you hardcode the package version-release
at the right of every changelog entry. Like:

* Tue Sep 02 2008 Christian Kreibich <christian> - 0.8-1
- Fix /usr/lib/ file list.


> $ rpmlint libpcapnav-devel-0.8-1.i386.rpm
> libpcapnav-devel.i386: W: no-documentation

This can be ignored.

> libpcapnav-devel.i386: E: library-without-ldconfig-postin
> /usr/lib/libpcapnav.so.0.0.0
> libpcapnav-devel.i386: E: library-without-ldconfig-postun
> /usr/lib/libpcapnav.so.0.0.0

These errors are only because your %files section is wrong.
The *.so.* files belong into the main package, not the -devel pkg.

> libpcapnav-devel.i386: W: no-version-dependency-on libpcapnav/libpcapnav-libs/liblibpcapnav 0.8

Most -devel packages must
Requires: %{name} = %{version}-%{release}

so main pkg and -devel pkg are kept in sync for any changes.

> libpcapnav-devel.i386: W: summary-ended-with-dot Development
> and documentation files for libpcapnav.

Most times "Summary" is not a full sentence. It's preferred to
not end it with a dot.


> $ rpmlint libpcapnav-0.8-1.i386.rpm 
> libpcapnav.i386: E: zero-length /usr/share/doc/libpcapnav-0.8/NEWS
> libpcapnav.i386: E: explicit-lib-dependency libpcap

See comment on -devel pkg. This must be versioned.


> %define version  0.8

Just do
  Version: 0.8
instead of redefining %version earlier.

> Release: 1

Using the %{dist} macro is not mandatory, but may be helpful if
using exactly the same src.rpm for several Fedora releases.
 -> Release: 1%{?dist}

> # Using FTP to get around SourceForge's habit of adding something after .tar.gz
> Source: ftp://ftp.sf.net/pub/sourceforge/netdude/%{name}-%{version}.tar.gz

There are recommendations on working http links somewhere in the
Fedora Packaging Wiki.


> Requires: libpcap

Delete this. There is an automatic dependency on the
libpcap SONAME added by rpmbuild. We rely on these dependencies.
Query the libpcapnav package to see.


> %description devel
> Static libraries, header files and documentation
> for libpcapnav

We don't build and include static libs unless there is very good
reason to do that. Libtool archives (*.la) should be deleted, too.


> %build

Build log contains many format string type warnings, e.g.
using %u for long int.

> if [ "$SMP" != "" ]; then

There is %{?_smp_mflags} that can simply be appended to "make".


> make prefix=$RPM_BUILD_ROOT%{prefix} install

Use: make DESTDIR=$RPM_BUILD_ROOT install



Can the tests be run in the %check section of the spec?
Should they pass?

$ ./run-tests.sh 
Running test 1 ... FAILED.


> %files
> %defattr(-,root,root,-)
> %doc AUTHORS COPYING ChangeLog NEWS README
> %{prefix}/lib/lib*.so.*

This would break on 64-bit multiarch platforms where there
is /usr/lib64 (and /usr/lib for 32-bit libs). Hence use:
%{_libdir}/*.so.*


> %files devel
> %defattr(-,root,root,-)
> %doc %{prefix}/share/gtk-doc/html/pcapnav

This is empty and only adds empty directories to the pkg.

> %{prefix}/lib/libpcapnav*

%_libdir
and include only the *.so symlink here:
%{_libdir}/*.so


> %{prefix}/include/pcapnav.h

%_includedir

> %{prefix}/bin/pcapnav-config

%_bindir


> drwxr-xr-x  /usr/share/gtk-doc/html/pcapnav
> drwxr-xr-x  /usr/share/gtk-doc/html/pcapnav/images


> $ pcapnav-config --cflags --libs
> -I/usr/include
> -L/usr/lib -lpcapnav -lpcap

The -I and -L here are bad as they alter the search order.
Can you make it not override default search paths with
default search paths?


Hope this catches all packaging issues. But only an updated src.rpm
will tell.

Comment 3 Michael Schwendt 2008-10-17 14:32:03 UTC
> License: BSD

It looks like MIT
http://www.opensource.org/licenses/mit-license.php
with an advertising-clause.

By comparison, this is the BSD licence:
http://www.opensource.org/licenses/bsd-license.php

Comment 4 Andreas Thienemann 2008-10-17 14:50:19 UTC
In case we want to shorten this a bit:

SPEC: http://home.bawue.de/~ixs/libpcapnav/libpcapnav.spec
SRPM: http://home.bawue.de/~ixs/libpcapnav/libpcapnav-0.8-1.fc9.src.rpm

Libpcapnav is a libpcap wrapper library that allows navigation to arbitrary
packets in a tcpdump trace file between reads, using timestamps or percentage
offsets. It was originally based on Vern Paxson's tcpslice tool.

[athienem@localhost ~]$ rpmlint  /var/lib/mock/fedora-rawhide-*/result/*.rpm
libpcapnav-debuginfo.i386: E: non-standard-dir-perm
/usr/src/debug/libpcapnav-0.8/src 02755
libpcapnav-debuginfo.x86_64: E: non-standard-dir-perm
/usr/src/debug/libpcapnav-0.8/src 02755
8 packages and 0 specfiles checked; 2 errors, 0 warnings.
[athienem@localhost ~]$ 
Autogenerated packages, setgid error is ignorable.

Comment 6 Mamoru TASAKA 2008-11-03 15:36:25 UTC
What is the status of this bug?

Comment 7 Michael Schwendt 2008-11-22 12:12:45 UTC
No comment from the package submitter.
I'm giving up for now. Also withdrawing the offer for sponsorship.

Comment 8 Jason Tibbitts 2008-11-25 03:17:47 UTC
Why return this to the queue?  Why not just close it and save someone else the hassle of dealing with it?