Bug 232816 - Review Request: libnetfilter_log - Netfilter logging userspace library
Review Request: libnetfilter_log - Netfilter logging userspace library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-03-18 07:36 EDT by Paul P Komkoff Jr
Modified: 2007-11-30 17:11 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-21 14:26:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jochen: fedora‑review+
petersen: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paul P Komkoff Jr 2007-03-18 07:36:37 EDT
Spec URL: http://palevo.sgu.ru/mockbuild/6/SPECS/libnetfilter_log.spec
SRPM URL: http://palevo.sgu.ru/mockbuild/6/SRPMS/libnetfilter_log-0.0.13-1.fc6.src.rpm
Description:
libnetfilter_log is a userspace library providing interface to packets that
have been logged by the kernel packet filter. It is is part of a system that
deprecates the old syslog/dmesg based packet logging.

libnetfilter_log has been previously known as libnfnetlink_log.

libnetfilter_log is used by ulogd2.
Comment 1 Jochen Schmitt 2007-03-19 14:11:24 EDT
Good:
+ Local build works fine.
+ Tar ball matches with upstream.
+ License Ok.
+ Mock build works fine.
+ Rpmlint of source rpm quite.
+ Rpmlint on binary rpm quite.

Bad:
- Source0 URL shows to wrong location.
- Devel RPM contains no docs.
- Devel RPM need Required to pkgconfig
Comment 3 Jochen Schmitt 2007-03-21 12:37:49 EDT
Good:
+ Tar ball matches with upstream.

Bad:
- You have put the BuildRequires to pkgconfig to the main package, but the
BuildRequires is required for the devel subpackage.
Comment 4 Paul P Komkoff Jr 2007-03-21 17:48:40 EDT
I am almost completely sure that pkgconfig is required to build this package as
a whole, that's why I added it to the BuildRequires of main package.
Comment 5 Paul P Komkoff Jr 2007-04-06 14:50:53 EDT
Jochen, can you say something here so I'd push this further? Thanks.
Comment 6 Jochen Schmitt 2007-05-22 11:07:30 EDT
Excluse me for the delay. I have make a text with a mock build and can tell you,
that you don't need pkgconfig as a BR for the main package.
Comment 7 Paul P Komkoff Jr 2007-05-23 02:43:26 EDT
Is there any way to skip building -devel subpackage?
If yes and we'll skip -devel, aren't we still need pkgconfig to do the build
(yes, we need, because the build step is common for both packages).

That said, is there any point in having BuildRequires for subpackages, at all?
Comment 8 Patrice Dumas 2007-05-23 04:52:21 EDT
(In reply to comment #3)
> Good:
> + Tar ball matches with upstream.
> 
> Bad:
> - You have put the BuildRequires to pkgconfig to the main package, but the
> BuildRequires is required for the devel subpackage.

BuildRequires are not for the main or the devel package, they are 
for the source package.

Here indeed pkgconfig is not needed as a BuildRequires because the
libnfnetlink libs is found with:
AC_CHECK_LIB(nfnetlink, nfnl_subsys_open, AC_MSG_RESULT(found),
AC_MSG_ERROR([libnfnetlink 0.0.16 or later needed]))
AC_CHECK_HEADER([libnfnetlink/linux_nfnetlink.h], [AC_MSG_RESULT([found])],
[AC_MSG_ERROR([libnfnetlink 0.0.16 or later needed])])

In general it is better to use pkgconfig instead, if available
(it is better for multilibs, for dependencies and allows to
have different subdirectories for different versions). So it 
would be better, in my opinion, to have something along:

PKG_CHECK_MODULES([NFNETLINK],[libnfnetlink >= 0.0.16],,
[
AC_CHECK_LIB(nfnetlink, nfnl_subsys_open, [NFNETLINK_LIBS=-lnfnetlink
AC_MSG_RESULT(found)], AC_MSG_ERROR([libnfnetlink 0.0.16 or later needed]))
AC_CHECK_HEADER([libnfnetlink/linux_nfnetlink.h], [AC_MSG_RESULT([found])],
[AC_MSG_ERROR([libnfnetlink 0.0.16 or later needed])])
])

And in 
src/Makefile.am, replace -lnfnetlink with NFNETLINK_LIBS, and add 
NFNETLINK_CFLAGS to AM_CFLAGS

This is to be tested, and for upstream, of course.

Also in top level Makefile.am 
LINKOPTS = -lnfnetlink
is unuseful

Also in src/Makefile.am 
AM_CFLAGS = -fPIC -Wall
is wrong, since -fPIC is added if needed by libtool and -Wall is
not portable.
Comment 9 Jochen Schmitt 2007-05-23 11:11:24 EDT
OK, you are right, so I can APPROVED your package.
Comment 10 Paul P Komkoff Jr 2007-05-26 18:50:12 EDT
Patrice, I'll add this to CVS first, then will fix the issues you've mentioned.
Comment 11 Paul P Komkoff Jr 2007-05-26 18:51:16 EDT
New Package CVS Request
=======================
Package Name: libnetfilter_log
Short Description: Netfilter logging userspace library
Owners: i@stingr.net
Branches: FC-6 F-7
InitialCC: 
Comment 12 Jens Petersen 2007-05-26 23:59:49 EDT
cvs done

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