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

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

Hello Michael,

Sorry for the delay in looking at your patch, and thanks for your
patch. A few questions below.

On Tue, 28 Nov 2023 12:36:34 +0100
Michael Nosthoff via buildroot <buildroot@buildroot.org> wrote:

> - drops support for proto2
> - fixes compatibility with protobuf >= 22.0

Is this fixing some build issues? We currently don't have protobuf >=
22.0 as far as I can see.

> - to be compatible with new protobuf versions c++17 is now required
>   when building with protoc (which we do for host) [0]
> - require host gcc >= 7 for c++17 support, propagate to depending packets

Why does this requirement of gcc >= 7 only applies to the host variant
of protobuf-c, not the target variant ?

> 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
>  	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
>  	select BR2_PACKAGE_LIBMICROHTTPD
>  	select BR2_PACKAGE_PROTOBUF_C
>  	help

We normally add a comment about Config.in dependencies, but admittedly
collectd/Config.in has none of these, so OK for this one.

> diff --git a/package/criu/Config.in b/package/criu/Config.in
> index ff3bf30229..c36569f0b1 100644
> --- a/package/criu/Config.in
> +++ b/package/criu/Config.in
> @@ -14,6 +14,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 dependency is not an architecture, so it should be on
BR2_PACKAGE_CRIU, and the Config.in comment should be updated as well:

comment "criu needs a glibc or musl toolchain w/ threads, gcc >= 8, headers >= 4.18, dynamic library, wchar"
        depends on BR2_PACKAGE_CRIU_ARCH_SUPPORTS
        depends on !BR2_TOOLCHAIN_GCC_AT_LEAST_8 || !BR2_TOOLCHAIN_HAS_THREADS \
                || !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_18 \
                || BR2_TOOLCHAIN_USES_UCLIBC \
                || BR2_STATIC_LIBS || !BR2_USE_WCHAR

This package uses both the target and host variants of protobuf-c, so
the question is whether the target variant needs target gcc >= 7 or not
(as stated above).

> diff --git a/package/kismet/Config.in b/package/kismet/Config.in
> index 7bde6c92af..0ba6637ef7 100644
> --- a/package/kismet/Config.in
> +++ b/package/kismet/Config.in
> @@ -11,6 +11,7 @@ config BR2_PACKAGE_KISMET
>  	depends on BR2_TOOLCHAIN_HAS_THREADS
>  	depends on BR2_USE_MMU # fork()
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS # protobuf-c
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # protobuf-c
>  	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_5 # C++14
>  	select BR2_PACKAGE_LIBPCAP
>  	select BR2_PACKAGE_PROTOBUF_C

Need an update of the Config.in comment.

> diff --git a/package/protobuf-c/Config.in b/package/protobuf-c/Config.in
> index d96cd7b382..aa2f640658 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
>  	help
>  	  Code generator and runtime libraries to use Protocol Buffers
>  	  from pure C (not C++).
>  
>  	  https://github.com/protobuf-c/protobuf-c
>  
> -comment "protobuf-c needs a toolchain w/ C++, threads"
> -	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS
> +comment "protobuf-c needs a toolchain w/ C++, threads and host gcc >= 7"

Instead of "and host gcc >= 7", just ", host gcc >= 7"

> +	depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_HOST_GCC_AT_LEAST_7
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS
> diff --git a/package/riemann-c-client/Config.in b/package/riemann-c-client/Config.in
> index 6c3c35caf8..5508fd7a6d 100644
> --- a/package/riemann-c-client/Config.in
> +++ b/package/riemann-c-client/Config.in
> @@ -3,6 +3,7 @@ config BR2_PACKAGE_RIEMANN_C_CLIENT
>  	depends on BR2_INSTALL_LIBSTDCPP # protobuf-c
>  	depends on BR2_TOOLCHAIN_HAS_THREADS # protobuf-c
>  	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS # protobuf-c
> +	depends on BR2_HOST_GCC_AT_LEAST_7 # protobuf-c

Needs an updated of the Config.in comment.

I was going to fix these myself, but then I wondered about the
target/host question above, and I preferred to get back to you on this.
Could you clarify this point, and submit a v2 that fixes the Config.in
comments?

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