Re: [Buildroot] [PATCH 1/1] package/mxt-app: new package

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

Hello,

On Tue, 19 Sep 2023 14:55:42 +0200
kazemi.ms@gmail.com wrote:

> From: Saeed Kazemi <kazemi.ms@gmail.com>
> 
> mxt-app is a utility for managing Atmel maXTouch touch controllers
> and other devices that support Atmel Object Based Protocol.
> 
> Signed-off-by: Saeed Kazemi <kazemi.ms@gmail.com>

Thanks for this contribution! This looks pretty good, but I have a few
comments below.

> ---
>  package/Config.in            |  1 +
>  package/mxt-app/Config.in    |  8 ++++++++
>  package/mxt-app/mxt-app.hash |  2 ++
>  package/mxt-app/mxt-app.mk   | 20 ++++++++++++++++++++
>  4 files changed, 31 insertions(+)
>  create mode 100644 package/mxt-app/Config.in
>  create mode 100644 package/mxt-app/mxt-app.hash
>  create mode 100644 package/mxt-app/mxt-app.mk

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

> diff --git a/package/mxt-app/Config.in b/package/mxt-app/Config.in
> new file mode 100644
> index 0000000000..7e8a5f4901
> --- /dev/null
> +++ b/package/mxt-app/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_MXT_APP
> +	bool "mxt-app"
> +	select BR2_PACKAGE_LIBUSB

You need to replicate the "depends on" from libusb:

        depends on BR2_TOOLCHAIN_HAS_THREADS # libusb
        depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 # libusb


> +	help
> +	  mxt-app is a utility for managing Atmel maXTouch touch controllers
> +	  and other devices that support Atmel Object Based Protocol.
> +
> +	  https://github.com/atmel-maxtouch/mxt-app/

And add a Config.in comment:

comment "mxt-app needs a toolchain w/ threads, gcc >= 4.9"
        depends on !BR2_TOOLCHAIN_HAS_THREADS || \
                !BR2_TOOLCHAIN_GCC_AT_LEAST_4_9


> diff --git a/package/mxt-app/mxt-app.hash b/package/mxt-app/mxt-app.hash
> new file mode 100644
> index 0000000000..abf20c728a
> --- /dev/null
> +++ b/package/mxt-app/mxt-app.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256  3639720ae1ea033bd982d68c5cab84432ce046fe14a66d9b2de07f115b642722  v1.36.tar.gz

We need the hash of the license file to be present.

> diff --git a/package/mxt-app/mxt-app.mk b/package/mxt-app/mxt-app.mk
> new file mode 100644
> index 0000000000..626cd352fc
> --- /dev/null
> +++ b/package/mxt-app/mxt-app.mk
> @@ -0,0 +1,20 @@
> +################################################################################
> +#
> +# mxt-app
> +#
> +################################################################################
> +
> +MXT_APP_VERSION = 1.36
> +MXT_APP_SOURCE = v$(MXT_APP_VERSION).tar.gz
> +MXT_APP_SITE = https://github.com/atmel-maxtouch/mxt-app/archive/refs/tags

Could you use the github macro instead?

MXT_APP_VERSION = 1.36
MXT_APP_SITE = $(call github,atmel-maxtouch,mxt-app,v$(MXT_APP_VERSION))

you will to adjust the hash file accordingly.

> +MXT_APP_LICENSE = Atmel License

The license is actually BSD-2-Clause

> +MXT_APP_LICENSE_FILES = LICENSE
> +MXT_APP_DEPENDENCIES = libusb
> +MXT_APP_INSTALL_STAGING = YES
> +MXT_APP_AUTORECONF = YES
> +MXT_APP_CONF_OPTS = --enable-debug

Why --enable-debug by default?

> +MXT_APP_CXXFLAGS = $(TARGET_CXXFLAGS)
> +MXT_APP_CONF_ENV = CXXFLAGS="$(MXT_APP_CXXFLAGS)"

I'm not sure what's the purpose of those two lines. CXXFLAGS are
already passed by the autotools-package infrastructure in the
environment of the configure script, so this seems redundant.

Could you rework the patch to take into account those suggestions, and
send an updated version?

Also, make sure to:

(1) Run "make check-package" to validate the coding style

(2) Use utils/test-pkg to validate that the package builds fine

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