Re: [Buildroot] [PATCH v2 3/5] package/optee-client: bump to version 3.19.0

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Étienne, All,

On 2022-12-09 08:51 +0100, Etienne Carriere spake thusly:
> Bumps OP-TEE client package version to OP-TEE release 3.19.0.
> 
> This package introduces a mandatory dependency on util-linux and
> pk-config packages that were made optional in commit [1], following
> 3.19.0 release tag. The dependency is related to new library teeacl
> for access control list based login identification. This change picks
> that commit and defines the dependency only when TEEACL library is
> to be embedded. The patch will be removed once we dump to the next
> OP-TEE release tag, as state by new BR2 boolean config switch
> BR2_PACKAGE_OPTEE_CLIENT_TEEACL.

We usually do not like to carry feature patches, but this one is
actually upstream, so that's OK-ish. So I kept it.

> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Squashes 2 commits from v1 series.
> - Adds OP-TEE Client fixup commit and new package config switch
>   BR2_PACKAGE_OPTEE_CLIENT_TEEACL to not mandate dependency on
>   pkg-config and usr-linux (libuuid).
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  ...condition-libteeacl-with-WITH_TEEACL.patch | 102 ++++++++++++++++++
>  package/optee-client/Config.in                |  10 ++
>  package/optee-client/optee-client.hash        |   4 +-
>  package/optee-client/optee-client.mk          |  12 ++-
>  4 files changed, 125 insertions(+), 3 deletions(-)
>  create mode 100644 package/optee-client/0001-libteeacl-condition-libteeacl-with-WITH_TEEACL.patch
> 
> diff --git a/package/optee-client/0001-libteeacl-condition-libteeacl-with-WITH_TEEACL.patch b/package/optee-client/0001-libteeacl-condition-libteeacl-with-WITH_TEEACL.patch
> new file mode 100644
> index 0000000000..53e7c4bf91
> --- /dev/null
> +++ b/package/optee-client/0001-libteeacl-condition-libteeacl-with-WITH_TEEACL.patch
> @@ -0,0 +1,102 @@
> +From 8e060441a50201b10aa61bc83c57d758bfe19cf1 Mon Sep 17 00:00:00 2001

Not sure where you got that sha1, but upstream commit is
bbdf665aba39c29a3ce7bd06e4554c62a416ebaa, so I actually backported that
instead.

> +From: Etienne Carriere <etienne.carriere@linaro.org>
> +Date: Thu, 10 Nov 2022 12:05:24 +0100
> +Subject: [PATCH] libteeacl: condition libteeacl with WITH_TEEACL
> +
> +Build and embed libteeacl upon WITH_TEEACL=1 (default configuration).
> +This configuration switch allows one to build OP-TEE client without
> +dependencies on pkg-config and libuuid  when OP-TEE ACL for
> +PKCS11 is not needed:
> + cmake -DWITH_TEEACL=0 ...
> +or
> + make WITH_TEEACL=0 ...
> +
> +With the comments below addressed, LGTM.
> +
> +Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> +Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
> +Reviewed-by: Eero Aaltonen <eero.aaltonen@vaisala.com>
> +Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

We also like to see a comment that this is an actual backport, so I
added it.

[--SNIP--]
> diff --git a/package/optee-client/Config.in b/package/optee-client/Config.in
> index cc7f176c77..3307a0ea83 100644
> --- a/package/optee-client/Config.in
> +++ b/package/optee-client/Config.in
> @@ -37,6 +37,16 @@ config BR2_PACKAGE_OPTEE_CLIENT_SUPP_PLUGINS
>  	help
>  	  Enable TEE supplicant plugin support.
>  
> +config BR2_PACKAGE_OPTEE_CLIENT_TEEACL
> +	bool "Enable TEE Access Control List login"
> +	default y

Why did you make it default to 'y'? It is a new feature, and you
backport a patch from upstream to make it optional, so there is no
reason to enable it by default, is there?

I dropped that, but if you believe it should defaut to y, then please
send a folow-up patch that explains why.

> +	depends on BR2_PACKAGE_UTIL_LINUX

I also changed that into a select, like we do almost eveywhere else. We
have a single location that depends on util-linux, but I just noticed it
is a spurious dependency, I'll send a patch to remove it.

> +	select BR2_PACKAGE_UTIL_LINUX_LIBS

That should usually not be needed, is only used to break a circular
dependency chan in special cases. I could not spot such a chain here, so
I dropped the _LIBS.

[--SNIP--]
> diff --git a/package/optee-client/optee-client.mk b/package/optee-client/optee-client.mk
> index 3fbbe9484c..d03791d3ad 100644
> --- a/package/optee-client/optee-client.mk
> +++ b/package/optee-client/optee-client.mk
> @@ -4,12 +4,16 @@
>  #
>  ################################################################################
>  
> -OPTEE_CLIENT_VERSION = 3.18.0
> +OPTEE_CLIENT_VERSION = 3.19.0
>  OPTEE_CLIENT_SITE = $(call github,OP-TEE,optee_client,$(OPTEE_CLIENT_VERSION))
>  OPTEE_CLIENT_LICENSE = BSD-2-Clause
>  OPTEE_CLIENT_LICENSE_FILES = LICENSE
>  OPTEE_CLIENT_INSTALL_STAGING = YES
>  
> +ifeq ($(BR2_PACKAGE_OPTEE_CLIENT_TEEACL),y)
> +OPTEE_CLIENT_EXT_DEPENDENCIES = host-pkgconf util-linux-libs

    $ make check-package
    package/optee-client/optee-client.mk:14: conditional override of variable OPTEE_CLIENT_EXT_DEPENDENCIES

> +endif
> +
>  OPTEE_CLIENT_CONF_OPTS = \
>  	-DCFG_TEE_FS_PARENT_PATH=$(BR2_PACKAGE_OPTEE_CLIENT_TEE_FS_PATH) \
>  	-DCFG_WERROR=OFF
> @@ -26,6 +30,12 @@ else
>  OPTEE_CLIENT_CONF_OPTS += -DCFG_TEE_SUPP_PLUGINS=OFF
>  endif
>  
> +ifeq ($(BR2_PACKAGE_OPTEE_CLIENT_TEEACL),y)
> +OPTEE_CLIENT_CONF_OPTS = -DWITH_TEEACL=ON
> +else
> +OPTEE_CLIENT_CONF_OPTS = -DWITH_TEEACL=OFF

    $ make check-package
    package/optee-client/optee-client.mk:34: conditional override of variable OPTEE_CLIENT_CONF_OPTS
    package/optee-client/optee-client.mk:36: conditional override of variable OPTEE_CLIENT_CONF_OPTS

Also, why did you add the condition twice, once to add the dependency
and one to add the conf opts? I squashed the two together.

Applied to master with all the above fixed, thanks.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot