Hi Thomas,

Thank you for your solid review.
Apparently I haven't done my homework very well. Sorry for that.

On Fri, Sep 08, 2023 at 10:57:04PM +0200, Thomas Petazzoni wrote:
> 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:
> 

[...]

> > +	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?

I'm pretty sure that I read that CRIU does not compile with musl
somewhere, but it seems like it does when I had a closer look.
I will remove it.


[...]

> 
> > +	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.

What I actually need is host-python-pip as it is used during the
installation step. Is there a way to only select the host-part from the 
PYTHON_PIP package?

> 
> > +	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.

Thank you.

[...]

> > +
> > +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?

I actually took those flags from the yocto recipe [1], but at least
_GNU_SOURCE seems to be in the package Makefile anyway.

I will remove __USE_GNU and _GNU-SOURCE.
Do not know about __WORDSIZE though.

[...]

> > +
> > +#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.

Sounds better.

[...]

> 
> 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.

Will do.

[...]

> 
> > +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.

Hrm, I had CROSS_COMPILE=$(TARGET_CROSS) at first, then I though I saw
that $(TARGET_MAKE_ENV) had it set.
Unfortunately, my last test was was on x86_64, so I did not notice it
was wrong.

I will put it back.

> 
> 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

[1] https://git.yoctoproject.org/meta-virtualization/tree/recipes-containers/criu/criu_git.bb

Thanks,
Marcus