Re: [Buildroot] [PATCH 1/2] package/criu: new package

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

Hello Marcus,

Thanks for your contribution! See some suggestions below.

On Fri,  8 Sep 2023 10:27:40 +0200
Marcus Folkesson <marcus.folkesson@gmail.com> wrote:

> Checkpoint/Restore In Userspace (CRIU), is a software tool for the Linux operating system to make it possible to freeze a running
> application and checkpoint it to persistent storage as a collection of files.

Commit log should be wrapped to ~80 columns.

> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  package/Config.in      |  1 +
>  package/criu/Config.in | 28 ++++++++++++++++++++++++++++
>  package/criu/criu.hash |  3 +++
>  package/criu/criu.mk   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 74 insertions(+)

An entry in the DEVELOPERS file should be added in this commit (not as
a separate patch).


> diff --git a/package/criu/Config.in b/package/criu/Config.in
> new file mode 100644
> index 0000000000..158e7ced57
> --- /dev/null
> +++ b/package/criu/Config.in
> @@ -0,0 +1,28 @@
> +menuconfig BR2_PACKAGE_CRIU
> +	bool "criu"
> +	depends on BR2_PACKAGE_PROTOBUF_ARCH_SUPPORTS
> +	depends on BR2_PACKAGE_LIBBSD_ARCH_SUPPORTS
> +	depends on BR2_INSTALL_LIBSTDCPP # protobuf
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # protobuf, libnl
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # protobuf
> +	depends on !BR2_TOOLCHAIN_USES_MUSL

Where does this dependency come from?

> +	depends on !BR2_STATIC_LIBS # protobuf, libbsd
> +	depends on BR2_USE_WCHAR #libbsd
> +	depends on BR2_USE_MMU #libcap

Spaces after # sign.

	depends on BR2_PACKAGE_HOST_PROTOBUF_ARCH_SUPPORTS # protobuf-c

is missing.

> +	select BR2_PACKAGE_HOST_PYTHON3_SSL
> +	select BR2_PACKAGE_PROTOBUF
> +	select BR2_PACKAGE_PROTOBUF_C
> +	select BR2_PACKAGE_LIBAIO
> +	select BR2_PACKAGE_LIBBSD
> +	select BR2_PACKAGE_LIBCAP
> +	select BR2_PACKAGE_LIBNET
> +	select BR2_PACKAGE_LIBNL
> +	select BR2_PACKAGE_PYTHON3
> +	select BR2_PACKAGE_PYTHON_PIP

It needs pip on the target? Seems odd.

> +	help
> +	  Checkpoint/Restore In Userspace (CRIU), is a software tool for the Linux operating system to make it possible to freeze a running 
> +	  application and checkpoint it to persistent storage as a collection of files.

This needs to be wrapped to the proper length. Run "make check-package"
to get the details.

Also we need the help text to end with a blank line, followed by the
URL of the upstream project.

> diff --git a/package/criu/criu.mk b/package/criu/criu.mk
> new file mode 100644
> index 0000000000..01b07e3f3f
> --- /dev/null
> +++ b/package/criu/criu.mk
> @@ -0,0 +1,42 @@
> +################################################################################
> +#
> +# CRIU
> +#
> +################################################################################
> +
> +CRIU_VERSION = 3.18
> +CRIU_SOURCE = criu-$(CRIU_VERSION).tar.gz
> +CRIU_SITE = https://github.com/checkpoint-restore/criu/archive/refs/tags/v$(CRIU_VERSION)
> +
> +CRIU_LICENSE = GPL-2.0
> +CRIU_LICENSE_FILES = COPYING
> +CRIU_DEPENDENCIES = host-pkgconf host-protobuf-c host-python3 host-python-pip libaio libbsd libcap libnet libnl protobuf protobuf-c python3

You can split this long line:

CRIU_DEPENDENCIES = \
	foo \
	bar \
	baz

> +
> +CRIU_CFLAGS = $(TARGET_CFLAGS)
> +CRIU_CFLAGS += -D__WORDSIZE -D__USE_GNU -D_GNU_SOURCE

Can be:

CRIU_CFLAGS = \
	$(TARGET_CFLAGS) \
	-D__WORDSIZE \
	-D...

However, this is odd. Why aren't those flags set by the package
Makefile?

> +
> +CRIU_MAKE_ENV = $(TARGET_MAKE_ENV)
> +CRIU_MAKE_ENV += WERROR=0

CRIU_MAKE_ENV = \
	$(TARGET_MAKE_ENV) \
	WERROR=0

> +
> +#Needed as it adds strange paths to the tool otherwise. E.g.  $CROSS_COMPILE/usr/bin/gcc
> +CRIU_MAKE_ENV += HOSTLD=ld
> +CRIU_MAKE_ENV += HOSTCC=gcc

Meh. Not sure to understand here. Maybe you want to pass
$(TARGET_CONFIGURE_OPTS), which does include HOSTCC, HOSTLD, and more.

> +
> +#x86_64 is treated as x86 in criu
> +ifeq ($(BR2_ARCH),x86_64)
> +	CRIU_MAKE_ENV += ARCH=x86

Don't indent this line.

> +else
> +	CRIU_MAKE_ENV += ARCH=$(BR2_ARCH)

Ditto.

In the Makefile:

ifneq ($(filter-out x86 arm aarch64 ppc64 s390 mips loongarch64,$(ARCH)),)
        $(error "The architecture $(ARCH) isn't supported")
endif

So you need a BR2_PACKAGE_CRIU_ARCH_SUPPORTS option that lists those
architectures only.

> +endif
> +
> +define CRIU_BUILD_CMDS
> +    rm -rf $(@D)/images/google/protobuf/descriptor.proto
> +    ln -s  $(STAGING_DIR)/usr/include/google/protobuf/descriptor.proto $(@D)/images/google/protobuf/descriptor.proto

Please indent those lines with one tab.

> +	$(CRIU_MAKE_ENV) $(MAKE) USERCFLAGS="$(CRIU_CFLAGS)" -C $(@D)

I don't understand how this can know which cross-compiler to use, you
are not passing it anywhere as far as I can see.

Could you have a look at addressing those comments and sending an
updated version?

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