Re: [Buildroot] [PATCH] Kea: new package - kea version 2.6.2

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Joseph,

Thanks for your contribution! See below a number of comments.

On Sat, 26 Apr 2025 10:50:00 +0300
"Joseph Zikusooka (ZIK)" <josephzik@gmail.com> wrote:

> Add Kea, an open-source Dynamic Host Configuration Protocol (DHCP)
> server developed by the Internet Systems Consortium (ISC).
> It's designed to provide dynamic IP address allocation and
> configuration for devices on a network. Kea is known for its
> modular architecture, allowing for easy extension and integration
> with other services.
> 
> Supersedes: a015291dc94b5497ac1fd60a2e4cf84fe3c82cb5

This is missing your Signed-off-by line, so unfortunately, we can't
apply it.

This "Supersedes:" line doesn't make sense, because
a015291dc94b5497ac1fd60a2e4cf84fe3c82cb5 isn't a reference that is
meaningful.

Also, the commit title should be:

	package/kea: new package

>  package/kea/Config.in | 17 +++++++++++++++++
>  package/kea/kea.hash  |  3 +++
>  package/kea/kea.mk    | 22 ++++++++++++++++++++++
>  3 files changed, 42 insertions(+)

You need to add an entry in the DEVELOPERS file.

> diff --git a/package/kea/Config.in b/package/kea/Config.in
> new file mode 100644
> index 0000000000..1b3bbe2e50
> --- /dev/null
> +++ b/package/kea/Config.in
> @@ -0,0 +1,17 @@
> +config BR2_PACKAGE_KEA
> +

This empty line shouldn't be there.

> +	bool "kea"
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on BR2_USE_MMU

Indicate in a comment why those dependencies are present.

> +	select BR2_PACKAGE_BOOST

Boost also depends on BR2_INSTALL_LIBSTDCPP.

> +	select BR2_PACKAGE_OPENSSL
> +	select BR2_PACKAGE_LOG4CPLUS

This one depends on a lot of stuff:

        depends on BR2_INSTALL_LIBSTDCPP
        depends on BR2_USE_WCHAR
        depends on BR2_TOOLCHAIN_HAS_THREADS
        depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # C++11
        depends on !BR2_TOOLCHAIN_HAS_GCC_BUG_64735 # std::future

So basically, you should have:

        depends on BR2_INSTALL_LIBSTDCPP # boost, log4cplus
        depends on BR2_USE_WCHAR # log4cplus, boost
        depends on BR2_TOOLCHAIN_HAS_THREADS # log4cplus, boost
        depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # log4cplus
        depends on !BR2_TOOLCHAIN_HAS_GCC_BUG_64735 # log4cplus

I'm not sure where your BR2_USE_MMU dependencies comes from. Maybe from
the kea code itself?

> +	help
> +	  DHCP implementation from Internet Systems Consortium, Inc. that features fully
> +	  functional DHCPv4, DHCPv6 and Dynamic DNS servers.
> +	  Both DHCP servers fully support server discovery, address assignment, renewal,
> +	  rebinding and release. The DHCPv6 server supports prefix delegation. Both
> +	  servers support DNS Update mechanism, using stand-alone DDNS daemon

Please wrap at 72 columns. Run "make check-package" to check the coding
style.

> +          https://www.isc.org/kea

You also need a Config.in comment to explain the dependencies. Along
the lines of:

comment "kea needs a toolchain w/ C++, wchar, threads, gcc >= 4.8"
        depends on !BR2_INSTALL_LIBSTDCPP || !BR2_USE_WCHAR || \
                !BR2_TOOLCHAIN_HAS_THREADS || !BR2_TOOLCHAIN_GCC_AT_LEAST_4_8

comment "kea needs a toolchain not affected by GCC bug 64735"
        depends on BR2_TOOLCHAIN_HAS_GCC_BUG_64735

> diff --git a/package/kea/kea.mk b/package/kea/kea.mk
> new file mode 100644
> index 0000000000..d589d2c1c2
> --- /dev/null
> +++ b/package/kea/kea.mk
> @@ -0,0 +1,22 @@
> +################################################################################
> +#
> +# kea
> +#
> +################################################################################
> +
> +KEA_VERSION = 2.6.2
> +KEA_SOURCE = kea-$(KEA_VERSION).tar.gz
> +KEA_SITE = https://dl.cloudsmith.io/public/isc/kea-2-6/raw/versions/$(KEA_VERSION)
> +KEA_LICENSE = MPL 2.0
> +KEA_LICENSE_FILES = COPYING
> +
> +KEA_DEPENDENCIES = host-pkgconf openssl boost log4cplus
> +
> +KEA_CONF_OPTS = "--with-openssl=$(STAGING_DIR)/usr" 
> +KEA_CONF_OPTS = "--with-log4cplus=$(STAGING_DIR)/usr"
> +#KEA_CONF_OPTS = "--with-boost-include=$(STAGING_DIR)/usr"

Comment lines are weird, if it's not needed, drop it. Also please write
this like this:

KEA_CONF_OPTS = \
	--with-openssl=$(STAGING_DIR)/usr \
	--with-log4cplus=$(STAGING_DIR)/usr

> +KEA_INSTALL_STAGING = YES
> +KEA_INSTALL_TARGET = YES

KEA_INSTALL_TARGET = YES is definitely not needed, as it's the default.

Why is KEA_INSTALL_STAGING = YES needed? Is kea installing some
libraries?

Could you fix those different issues, and come back with an updated
patch? Make sure to name it [PATCH v2], and provide a changelog. See
https://buildroot.org/downloads/manual/manual.html#submitting-patches
for some good description on how to contribute.

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot