Re: [Buildroot] [PATCH 8/9] package/libpisp: add new package

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

Hello Quentin,

Thanks for this new package! I have a number of comments/requests.

First, very minor, the commit title should be "package/libpisp: new
package", not "add new package".

On Fri, 04 Apr 2025 18:47:39 +0200
Quentin Schulz <foss+buildroot@0leil.net> wrote:

>  package/Config.in            |  1 +
>  package/libpisp/Config.in    | 22 ++++++++++++++++++++++
>  package/libpisp/libpisp.hash | 11 +++++++++++
>  package/libpisp/libpisp.mk   | 33 +++++++++++++++++++++++++++++++++
>  4 files changed, 67 insertions(+)

Could you add an entry in the DEVELOPERS file for this new package?


> diff --git a/package/libpisp/Config.in b/package/libpisp/Config.in
> new file mode 100644
> index 0000000000000000000000000000000000000000..8586c31f42914a219538e04fe93b62de38e0daab
> --- /dev/null
> +++ b/package/libpisp/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_LIBPISP
> +	bool "libpisp"
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_8 # C++17
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on BR2_PACKAGE_JSON_FOR_MODERN_CPP

This one should be a "select", not a depends on.

> +	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_5_0 # V4L2_BUF_TYPE_META_OUTPUT
> +	help
> +	  A helper library to generate run-time configuration for the
> +	  Raspberry Pi ISP (PiSP), consisting of the Frontend and
> +	  Backend hardware components.
> +
> +	  https://github.com/raspberrypi/libpisp
> +
> +if BR2_PACKAGE_LIBPISP
> +
> +config BR2_PACKAGE_LIBPISP_LOGGING
> +	bool "enable logging"
> +	depends on BR2_PACKAGE_BOOST_LOG

This should be a select BR2_PACKAGE_BOOST + select
BR2_PACKAGE_BOOST_LOG + propagate the dependencies of boost.

> +	help
> +	  Enable logging
> +endif

Here we need a Config.in comment like:

comment "libpisp needs a toolchain w/ C++, gcc >= 8, threads, headers >= 5.0"
	depends on !BR2_INSTALL_LIBSTDCPP ||
		!BR2_TOOLCHAIN_GCC_AT_LEAST_8 ||
		!BR2_TOOLCHAIN_HAS_THREADS ||
		!BR2_TOOLCHAIN_HEADERS_AT_LEAST_5_0


> new file mode 100644
> index 0000000000000000000000000000000000000000..66a44c0d14c52bf64cecd8eea77b7c3237a815a4
> --- /dev/null
> +++ b/package/libpisp/libpisp.mk
> @@ -0,0 +1,33 @@
> +################################################################################
> +#
> +# libpisp
> +#
> +################################################################################
> +
> +LIBPISP_VERSION = 1.2.0
> +# The manually generated tarball has version.gen in it which is required for
> +# version detection outside of git repos
> +LIBPISP_SITE = https://github.com/raspberrypi/libpisp/releases/download/v$(LIBPISP_VERSION)
> +LIBPISP_SOURCE = libpisp-$(LIBPISP_VERSION).tar.xz
> +LIBPISP_LICENSE = BSD-2-Clause

BSD-2-Clause, but there's the text of many other licenses? Could you
clarify the license terms? How can it be BSD-2c if it contains GPLv2,
GPLv2+ code?

> +LIBPISP_LICENSE_FILES = \
> +	LICENSE \
> +	LICENSES/BSD-2-Clause.txt \
> +	LICENSES/CC0-1.0.txt \
> +	LICENSES/GPL-2.0-only.txt \
> +	LICENSES/GPL-2.0-or-later.txt \
> +	LICENSES/Linux-syscall-note.txt \
> +	LICENSES/MIT.txt
> +LIBPISP_CONF_OPTS = \
> +	-Dauto_features=disabled
> +LIBPISP_INSTALL_STAGING = YES
> +LIBPISP_DEPENDENCIES = \
> +	host-pkgconf \
> +	json-for-modern-cpp
> +
> +ifeq ($(BR2_PACKAGE_LIBPISP_LOGGING),y)
> +LIBPISP_CONF_OPTS += -Dlogging=enabled
> +LIBPISP_DEPENDENCIES += boost
> +endif

Otherwise, looks good! Could you spin a v2? 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