Re: [Buildroot] [PATCH] package/minijail: new package

[Thomas Petazzoni <thomas.petazzoni@bootlin.com>]

Hello José,

Thanks for this new iteration! Comments below.

On Thu, 13 Jan 2022 12:05:06 +0200
José Pekkarinen <jose.pekkarinen@unikie.com> wrote:

> This patch adds package minijail
> 
> Minijail depends in a toolchain different from
> uclibc thanks to it's lack of support for prlimits.
> 
> Signed-off-by: José Pekkarinen <jose.pekkarinen@unikie.com>
> ---
> [ v1 -> v2 ]
> - Fixed hash file
> - Fixed static assert patch
> - Depend in toolchain distinct of uclibc
> - Remove redundant host libpcap dependency
> - Remove redundant parenthesis on MINIJAIL_BUILD_CMDS

If this is v2, your patch should have been generated with "git
format-patch -v2", so that its title is [PATCH v2] and not just [PATCH].

> diff --git a/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> new file mode 100644
> index 0000000000..ff85995114
> --- /dev/null
> +++ b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> @@ -0,0 +1,35 @@
> +From 8a6d5a1c48b85fb49f0d68ec31ecc51fd22e7201 Mon Sep 17 00:00:00 2001
> +From: =?UTF-8?q?Jos=C3=A9=20Pekkarinen?= <jose.pekkarinen@unikie.com>
> +Date: Wed, 12 Jan 2022 17:09:27 +0200
> +Subject: [PATCH] Substitute static_assert with _Static_assert
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +Substitute static_assert with _Static_assert
> +
> +static_assert behaves differently for uclibc
> +toolchains. Substituting it with the standard
> +_Static_assert builds on all toolchains tested.

So this is fixing a problem affecting uClibc toolchains, and below you
exclude uClibc toolchains ? Does it make sense to have this patch ?

> diff --git a/package/minijail/Config.in b/package/minijail/Config.in
> new file mode 100644
> index 0000000000..24d307ed54
> --- /dev/null
> +++ b/package/minijail/Config.in
> @@ -0,0 +1,12 @@
> +config BR2_PACKAGE_MINIJAIL
> +	bool "minijail"
> +	depends on !BR2_STATIC_LIBS # dlopen()
> +	depends on !BR2_TOOLCHAIN_USES_UCLIBC

Please add a comment on top of this that explains why uClibc toolchains
are excluded (prlimit not implemented).

> +	select BR2_PACKAGE_LIBCAP

You need to replicate:

	depends on BR2_USE_MMU

from the list of libcap dependencies.

> +	help
> +	  Minijail is a sandboxing tool maintained by google.
> +
> +	  https://google.github.io/minijail/
> +
> +comment "minijail needs a glibc or musl toolchain with dynamic library support"

and also have it here.

> +	depends on BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_UCLIBC
> diff --git a/package/minijail/minijail.hash b/package/minijail/minijail.hash
> new file mode 100644
> index 0000000000..d9f497a86c
> --- /dev/null
> +++ b/package/minijail/minijail.hash
> @@ -0,0 +1,5 @@
> +# Locally computed from https://github.com/google/minijail/releases/
> +sha256  1ee5a5916491a32c121c7422b4d8c16481c0396a3acab34bf1c44589dcf810ae  linux-v17.tar.gz
> +
> +# Locally computed
> +sha256  c6f439c5cf07263f71f01d29b79c79172ee529088e51ab434b22baad0988fe57  LICENSE
> diff --git a/package/minijail/minijail.mk b/package/minijail/minijail.mk
> new file mode 100644
> index 0000000000..78898865fb
> --- /dev/null
> +++ b/package/minijail/minijail.mk
> @@ -0,0 +1,28 @@
> +################################################################################
> +#
> +# minijail
> +#
> +################################################################################
> +
> +MINIJAIL_VERSION = linux-v17
> +MINIJAIL_SOURCE = $(MINIJAIL_VERSION).tar.gz
> +MINIJAIL_SITE = "https://github.com/google/minijail/archive/refs/tags"

Please use the following lines instead:

MINIJAIL_VERSION = 17
MINIJAIL_SITE = $(call github,google,minijail,linux-v$(MINIJAIL_VERSION))

You will have to adjust the .hash file accordingly.

> +MINIJAIL_LICENSE = BSD-Style

This is not a license. See https://spdx.org/licenses/ for the list of
license identifiers that are valid.

> +MINIJAIL_LICENSE_FILES = LICENSE
> +MINIJAIL_DEPENDENCIES=libcap

Spaces around "=".

> +define MINIJAIL_BUILD_CMDS
> +	cd $(@D); \
> +	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/$(d) CC="$(TARGET_CC)"

No need to cd into $(@D), since you run make with -C $(@D). Please drop
the $(d) which is an empty variable. Also please use
$(TARGET_CONFIGURE_OPTS). So something like this:

	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D)

I'm pretty sure I already made this comment on a previous version, or
perhaps on another package contributed by you.

Could you take into account those comments and send a v3 ? It should be
good to merge then. 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