[2/2] netsniff-ng:new package

Signed-off-by: Joris Lijssens <joris.lijssens@gmail.com>
---
 package/Config.in                  |  1 +
 package/netsniff-ng/Config.in      | 22 ++++++++++++++++++++++
 package/netsniff-ng/netsniff-ng.mk | 30 ++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 package/netsniff-ng/Config.in
 create mode 100644 package/netsniff-ng/netsniff-ng.mk

Hi Joris,

On 09-11-15 10:03, Joris Lijssens wrote:
> Signed-off-by: Joris Lijssens <joris.lijssens@gmail.com>
> ---
>  package/Config.in                  |  1 +
>  package/netsniff-ng/Config.in      | 22 ++++++++++++++++++++++
>  package/netsniff-ng/netsniff-ng.mk | 30 ++++++++++++++++++++++++++++++

 Same comment about the .hash file.

>  3 files changed, 53 insertions(+)
>  create mode 100644 package/netsniff-ng/Config.in
>  create mode 100644 package/netsniff-ng/netsniff-ng.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index e0b42c0..0a80afb 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -88,6 +88,7 @@ endif
>  	source "package/mcelog/Config.in"
>  	source "package/memstat/Config.in"
>  	source "package/netperf/Config.in"
> +	source "package/netsniff-ng/Config.in"
>  	source "package/oprofile/Config.in"
>  	source "package/pax-utils/Config.in"
>  	source "package/pv/Config.in"
> diff --git a/package/netsniff-ng/Config.in b/package/netsniff-ng/Config.in
> new file mode 100644
> index 0000000..448e853
> --- /dev/null
> +++ b/package/netsniff-ng/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_NETSNIFF_NG
> +	bool "netsniff-ng"
> +	select BR2_PACKAGE_LIBNL
> +	select BR2_PACKAGE_LIBPCAP
> +	select BR2_PACKAGE_LIBCLI
> +	select BR2_PACKAGE_LIBNETFILTER_CONNTRACK
> +	select BR2_PACKAGE_LIBURCU

 You also have to take along the arch dependencies of liburcu:

	depends on BR2_arm || BR2_armeb || BR2_aarch64 || BR2_i386 || BR2_powerpc ||
BR2_x86_64

 But actually, all of these dependencies are optional, i.e. the corresponding
features will be disabled but the package still builds fine. In that case, we
handle it purely in the .mk file, see below.

> +	select BR2_PACKAGE_LIBNET
> +	depends on BR2_TOOLCHAIN_USES_GLIBC

 Where does this dependency come from? Mention it in a comment here and/or in
the commit log.

> +	depends on BR2_TOOLCHAIN_HAS_THREADS

 Note to other reviewers: this package unconditionally uses threads so it's not
an inherited dependency.

> +	depends on !BR2_TOOLCHAIN_HAS_GCC_BUG_58854 # liburcu
> +	help
> +	  netsniff-ng is a free, performant Linux network analyzer and
> +	  networking toolkit. If you will, the Swiss army knife for
> +	  network packets.
> +
> +comment "netsniff-ng needs an (e)glibc toolchain w/ threads"
> +	depends on !BR2_TOOLCHAIN_HAS_THREADS || !BR2_TOOLCHAIN_USES_GLIBC
> +	depends on !BR2_TOOLCHAIN_HAS_GCC_BUG_58854

 Arch dependencies have to be repeated here.

> +
> +comment "netsniff-ng needs a toolchain not affected by GCC bug 58854"
> +	depends on BR2_TOOLCHAIN_HAS_GCC_BUG_58854
> diff --git a/package/netsniff-ng/netsniff-ng.mk b/package/netsniff-ng/netsniff-ng.mk
> new file mode 100644
> index 0000000..6f755a1
> --- /dev/null
> +++ b/package/netsniff-ng/netsniff-ng.mk
> @@ -0,0 +1,30 @@
> +################################################################################
> +#
> +# netsniff-ng
> +#
> +################################################################################
> +
> +NETSNIFF_NG_VERSION = v0.5.9
> +NETSNIFF_NG_SITE = $(call github,netsniff-ng,netsniff-ng,$(NETSNIFF_NG_VERSION))
> +NETSNIFF_NG_LICENSE = GPLv2

 I double-checked that this really is the case. Many source files don't have a
license header, but the ones that do are really v2-only.

> +NETSNIFF_NG_LICENSE_FILES = README

 Should be COPYING

 host-pkgconf is a dependency. You should also add all the selects from
Config.in as _DEPENDENCIES here. But as I said, they're not obligatory
dependencies. So this should be the dependency list:

NETSNIFF_NG_DEPENDENCIES = host-pkgconf

ifeq ($(BR2_PACKAGE_LIBNL),y)
NETSNIFF_NG_DEPENDENCIES += libnl
endif

 There is also an optional dependency on ncurses, geoip, zlib.

 Also host-flex and host-bison are optional dependencies. For these, we don't
have a good way to deal with them, so I propose to add them unconditionally.
Mention that in the commit log and/or a comment.

> +
> +define NETSNIFF_NG_CONFIGURE_CMDS
> +		(cd $(@D); \

 One tab too many here.

> +		$(TARGET_CONFIGURE_ARGS) \
> +		$(TARGET_CONFIGURE_OPTS) \
> +		./configure \
> +		--prefix=$(TARGET_DIR)/usr \
> +	)

 The brackets around it are not needed in fact.

 Unfortunately, this manually written configure script has some deficiencies. It
will look for nacl in /usr/include/nacl, so if it happens to be installed there,
we'll get a wrong build. This can be fixed by adding NACL_INC_DIR=/dev/null and
NACL_LIB_DIR=/dev/null to NETSNIFF_NG_CONF_ENV (and adding that to the _CMDS of
course).

 I've marked both patches as 'Changes requested' in patchwork, so we will forget
about them unless you send a new version.

 Regards,
 Arnout


> +endef
> +
> +define NETSNIFF_NG_BUILD_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D)
> +endef
> +
> +define NETSNIFF_NG_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) $(TARGET_CONFIGURE_OPTS) \
> +		PREFIX=$(TARGET_DIR)/usr ETCDIR=$(TARGET_DIR)/etc install -C $(@D)
> +endef
> +
> +$(eval $(generic-package))
>

Dear Joris Lijssens,

On Mon,  9 Nov 2015 10:03:32 +0100, Joris Lijssens wrote:
> Signed-off-by: Joris Lijssens <joris.lijssens@gmail.com>
> ---
>  package/Config.in                  |  1 +
>  package/netsniff-ng/Config.in      | 22 ++++++++++++++++++++++
>  package/netsniff-ng/netsniff-ng.mk | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
>  create mode 100644 package/netsniff-ng/Config.in
>  create mode 100644 package/netsniff-ng/netsniff-ng.mk

I've applied your patch after doing a number of changes:

    [Thomas:
     - add comment in Config.in to explain why this package depends on
       glibc, and specifically why it doesn't build for uClibc and musl.
     - add hash file.
     - add 'COPYING' to the list of LICENSE_FILES
     - add themissing <pkg>_DEPENDENCIES variable in the .mk file, to make
       sure dependencies are built before netsniff-ng.
     - add comment in the .mk file explain why we're using the
       generic-package infrastructure even if the build procedure is
       essentially ./configure, make, make install.
     - fix indentation in NETSNIFF_NG_CONFIGURE_CMDS.]

Though I believe you should look into the comments made by Arnout,
since some of them remain valid even after my changes, and it would be
great if you could address them through follow-up patches.

Best regards,

Thomas