Re: [Buildroot] [PATCH v2] package/protobuf-c: bump to version 1.5.0

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

Hello Michael,

On Fri, 19 Jan 2024 08:29:27 +0100
Michael Nosthoff via buildroot <buildroot@buildroot.org> wrote:

> - drops support for proto2
> - fixes compatibility with protobuf >= 22.0 (which we did not yet bump)
> - to be compatible with new protobuf versions c++17 is now required
>   when building with protoc (which we only do for the host package) [0]
>   hence require host gcc >= 7 for c++17 support, propagate to depending packets
> 
> [0] https://github.com/protobuf-c/protobuf-c/pull/673
> 
> Signed-off-by: Michael Nosthoff <buildroot@heine.tech>

Thanks, I've applied your patch, but after doing a few more changes.
See below.

First, you changed the hash of the license file, and that needs to be
explained in the commit log so that we have the visibility that you did
check why the license file has changed. Indeed, the sole reason for
having hashes for the license file is to detect changes in the
licensing terms. If we 'blindly' update the hash, we miss the point :-)

> diff --git a/package/collectd/Config.in b/package/collectd/Config.in
> index d3b686771d..145b9a2a3a 100644
> --- a/package/collectd/Config.in
> +++ b/package/collectd/Config.in
> @@ -738,6 +738,7 @@ config BR2_PACKAGE_COLLECTD_RIEMANN
>  	# riemann-c-client -> protobuf-c
>  	depends on BR2_INSTALL_LIBSTDCPP
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # protobuf-c
>  	select BR2_PACKAGE_RIEMANN_C_CLIENT
>  	select BR2_PACKAGE_LIBTOOL

Needed a Config.in comment (which indeed didn't exist until now for
existing dependencies).

>  	help
> @@ -772,6 +773,7 @@ config BR2_PACKAGE_COLLECTD_WRITEPROMETHEUS
>  	bool "write_prometheus"
>  	depends on BR2_INSTALL_LIBSTDCPP # protobuf-c
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS # protobuf-c
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # protobuf-c

Needed an update to the existing Config.in comment.

> diff --git a/package/criu/Config.in b/package/criu/Config.in
> index 4c295acf4f..d223524eee 100644
> --- a/package/criu/Config.in
> +++ b/package/criu/Config.in
> @@ -12,6 +12,7 @@ config BR2_PACKAGE_CRIU_ARCH_SUPPORTS
>  	# BE/LE endian issues.
>  	depends on BR2_USE_MMU # libcap
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS # protobuf-c
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # protobuf-c

This shouldn't be in BR2_PACKAGE_CRIU_ARCH_SUPPORTS, because it's not
an architecture dependency. It should be on BR2_PACKAGE_CRIU, with the
appropriate Config.in comment update.

> diff --git a/package/kismet/Config.in b/package/kismet/Config.in
> index 7bde6c92af..ea2b33fca7 100644
> --- a/package/kismet/Config.in
> +++ b/package/kismet/Config.in
> @@ -1,8 +1,9 @@
> -comment "kismet needs a toolchain w/ threads, C++, gcc >= 5"
> +comment "kismet needs a toolchain w/ threads, C++, gcc >= 5, host-gcc >= 7"

                                                                ^^^^^ we use "host gcc" everywhere, without the dash

> diff --git a/package/protobuf-c/Config.in b/package/protobuf-c/Config.in
> index d96cd7b382..56b58afe99 100644
> --- a/package/protobuf-c/Config.in
> +++ b/package/protobuf-c/Config.in
> @@ -4,12 +4,13 @@ config BR2_PACKAGE_PROTOBUF_C
>  	depends on BR2_TOOLCHAIN_HAS_THREADS
>  	# host-protobuf only builds on certain architectures
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # c++17

I've extended the comment to indicate that it is for the host package.
Otherwise, the next person coming will wonder why for C++17 we depends
on host gcc >= 7, and not target gcc >= 7.

Applied with those fixes. Thanks a lot for your contribution!

Best regards,

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