Re: [Buildroot] [PATCH v5 2/9] package/libnetconf2: bump version to 3.5.5

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Heiko Thiery <heiko.thiery@gmail.com>
Cc: buildroot@buildroot.org, "Fiona Klute" <fiona.klute@gmx.de>,
	"Jan Kundrát" <jan.kundrat@cesnet.cz>
Subject: Re: [Buildroot] [PATCH v5 2/9] package/libnetconf2: bump version to 3.5.5
Date: Mon, 3 Feb 2025 23:41:00 +0100	[thread overview]
Message-ID: <20250203234100.6c0a6835@windsurf> (raw)
In-Reply-To: <20250114072947.284965-3-heiko.thiery@gmail.com>

Hello Heiko,

On Tue, 14 Jan 2025 08:29:44 +0100
Heiko Thiery <heiko.thiery@gmail.com> wrote:

> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>

I wanted to finally apply this, but unfortunately it's still not good :/

First of all, your commit log is entirely empty, with zero explanations
about the changes, even though there are non-trivial changes. Previous
iterations had some explanations, but this iteration has none.

> -LIBNETCONF2_VERSION = 2.1.28
> +LIBNETCONF2_VERSION = 3.5.5
>  LIBNETCONF2_SITE = $(call github,CESNET,libnetconf2,v$(LIBNETCONF2_VERSION))
>  LIBNETCONF2_INSTALL_STAGING = YES
>  LIBNETCONF2_LICENSE = BSD-3-Clause
>  LIBNETCONF2_LICENSE_FILES = LICENSE
> -LIBNETCONF2_DEPENDENCIES = libyang
> +LIBNETCONF2_DEPENDENCIES = libyang openssl

In fact openssl is NOT a mandatory dependency of libnetconf2. If you
look at the CMakeLists.txt, OpenSSL is only needed when
ENABLE_SSH_TLS=ON (and still, when mbedtls is not available).

Yes, it fails to build without OpenSSL when ENABLE_SSH_TLS=OFF, but
that's a libnetconf2 that should be reported/fixed.

>  HOST_LIBNETCONF2_DEPENDENCIES = host-libyang
> 
>  LIBNETCONF2_CONF_OPTS = \
>  	-DENABLE_TESTS=OFF \
>  	-DENABLE_VALGRIND_TESTS=OFF
> 
> -ifeq ($(BR2_PACKAGE_LIBSSH_SERVER), y)
> -LIBNETCONF2_CONF_OPTS += -DENABLE_SSH=ON
> -LIBNETCONF2_DEPENDENCIES += libssh
> +ifeq ($(BR2_PACKAGE_LIBSSH_SERVER)$(BR2_PACKAGE_LIBCURL),yy)
> +LIBNETCONF2_CONF_OPTS += -DENABLE_SSH_TLS=ON
> +LIBNETCONF2_DEPENDENCIES += libssh libcurl
>  else
> -LIBNETCONF2_CONF_OPTS += -DENABLE_SSH=OFF
> -endif
> -
> -ifeq ($(BR2_PACKAGE_LIBOPENSSL), y)
> -LIBNETCONF2_CONF_OPTS += -DENABLE_TLS=ON
> -LIBNETCONF2_DEPENDENCIES += openssl
> -else
> -LIBNETCONF2_CONF_OPTS += -DENABLE_TLS=OFF
> +LIBNETCONF2_CONF_OPTS += -DENABLE_SSH_TLS=OFF
>  endif

The proper logic that matches what's in the CMakeLists.txt is more like this:

ifeq ($(BR2_PACKAGE_OPENSSL)$(BR2_PACKAGE_MBEDTLS):$(BR2_PACKAGE_LIBSSH)$(BR2_PACKAGE_LIBCURL),y:yy)
LIBNETCONF2_CONF_OPTS += -DENABLE_SSH_TLS=ON
LIBNETCONF2_DEPENDENCIES += libssh libcurl
ifeq ($(BR2_PACKAGE_MBEDTLS),y)
LIBNETCONF2_DEPENDENCIES += mbedtls
else
LIBNETCONF2_DEPENDENCIES += openssl
endif
else
LIBNETCONF2_CONF_OPTS += -DENABLE_SSH_TLS=OFF
endif

However, as that's tricky, I think we could also accept a sub-option in
libnetconf2/Config.in:

config BR2_PACKAGE_LIBNETCONF2_SSH_TLS
	bool "SSH/TLS support"
	select BR2_PACKAGE_OPENSSL if !BR2_PACKAGE_MBEDTLS
	select BR2_PACKAGE_LIBCURL
	select BR2_PACKAGE_LIBSSH

>  ifeq ($(BR2_PACKAGE_LIBXCRYPT),y)
> @@ -37,8 +30,7 @@ endif
>  HOST_LIBNETCONF2_CONF_OPTS = \
>  	-DENABLE_TESTS=OFF \
>  	-DENABLE_VALGRIND_TESTS=OFF \
> -	-DENABLE_SSH=OFF \
> -	-DENABLE_TLS=OFF
> +	-DENABLE_TLS_SSH=OFF

This option doesn't exist, did you mean ENABLE_SSH_TLS=OFF ?

Thanks!

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