Re: [Buildroot] [PATCH v1 1/1] package/crio: new package

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

Hello Christian,

Thanks for the patch. See below some review.

On Thu, 11 May 2023 18:40:56 -0700
Christian Stewart via buildroot <buildroot@buildroot.org> wrote:

>  package/Config.in      |  1 +
>  package/crio/Config.in | 54 ++++++++++++++++++++++++++++
>  package/crio/crio.hash |  3 ++
>  package/crio/crio.mk   | 82 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 140 insertions(+)

Entry in DEVELOPERS file missing.

> diff --git a/package/crio/Config.in b/package/crio/Config.in
> new file mode 100644
> index 0000000000..35a38c587e
> --- /dev/null
> +++ b/package/crio/Config.in
> @@ -0,0 +1,54 @@
> +config BR2_PACKAGE_CRIO
> +	bool "crio"
> +	depends on BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS
> +	depends on BR2_PACKAGE_HOST_GO_TARGET_CGO_LINKING_SUPPORTS
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_4 # iproute2, __kernel_{u,}long_t
> +	depends on !BR2_TOOLCHAIN_USES_UCLIBC # no fexecve

uClibc has fexecve() now

> +	depends on BR2_USE_MMU # libgpgme, iproute2, fork()

	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libgpgme

is missing

> +	select BR2_PACKAGE_IPROUTE2
> +	select BR2_PACKAGE_IPTABLES

Neither of these are referenced in the .mk file. Are these runtime
dependencies? If so, please indicate this via a comment.

For this kind of package, a runtime test in support/testing/ would be
very good. You can ask Julien Olivain for help here :-)

> +config BR2_PACKAGE_CRIO_DRIVER_BTRFS
> +	bool "btrfs filesystem driver"
> +	depends on BR2_USE_MMU # btrfs-progs
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # btrfs-progs
> +	select BR2_PACKAGE_BTRFS_PROGS
> +	help
> +	  Build the btrfs filesystem driver.

Config.in comment missing.

> +
> +config BR2_PACKAGE_CRIO_DRIVER_DEVICEMAPPER
> +	bool "devicemapper filesystem driver"
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # lvm2
> +	depends on BR2_USE_MMU # lvm2
> +	depends on !BR2_STATIC_LIBS # lvm2
> +	select BR2_PACKAGE_LVM2
> +	help
> +	  Build the devicemapper filesystem driver.

Ditto.

> +
> +config BR2_PACKAGE_CRIO_DRIVER_OSTREE
> +	bool "ostree storage driver"
> +	depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libostree, libgpgme, libgpg-error
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # libostree, libglib2
> +	depends on BR2_USE_WCHAR # libostree, libglib2
> +	depends on BR2_USE_MMU # libostree, e2fsprogs, libglib2, libgpgme

Don't need to duplicate the whole set of comments, it can be just:

	depends on .... # libostree

because all what you select is libostree

> +	# doesn't build with musl due to lack of TEMP_FAILURE_RETRY()

don't need to replicate this comment here

> +	depends on !BR2_TOOLCHAIN_USES_MUSL # libostree

This is enough.

> +	select BR2_PACKAGE_LIBOSTREE
> +	help
> +	  Build the ostree storage driver.

Config.in comment needed.

> +
> +endif
> +
> +comment "crio needs a glibc or musl toolchain w/ threads"
> +	depends on BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS && \
> +		BR2_PACKAGE_HOST_GO_TARGET_CGO_LINKING_SUPPORTS
> +	depends on !BR2_TOOLCHAIN_HAS_THREADS || BR2_TOOLCHAIN_USES_UCLIBC

Needs to be adjusted as per the above comments.

> +################################################################################
> +#
> +# crio
> +#
> +################################################################################
> +
> +CRIO_VERSION = 1.27.0
> +CRIO_SITE = $(call github,cri-o,cri-o,v$(CRIO_VERSION))
> +CRIO_LICENSE = Apache-2.0
> +CRIO_LICENSE_FILES = LICENSE
> +
> +CRIO_CPE_ID_VENDOR = kubernetes
> +CRIO_CPE_ID_PRODUCT = cri-o
> +
> +CRIO_BUILD_TARGETS = cmd/crio cmd/crio-status
> +CRIO_DEPENDENCIES += libgpgme

Changed += to = here.

> +define CRIO_BUILD_PINNS
> +	$(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" CFLAGS="$(TARGET_CFLAGS)" \
> +		LDFLAGS="$(TARGET_LDFLAGS)" STRIP="$(TARGET_STRIP)" \

Would $(TARGET_CONFIGURE_OPTS) work here to replace CC, CFLAGS, LDFLAGS
and STRIP?

> +		-C $(@D)/pinns ../bin/pinns
> +endef
> +CRIO_POST_BUILD_HOOKS += CRIO_BUILD_PINNS

Perhaps a short comment above define CRIO_BUILD_PINNS to explain why it
is needed, and not done by the normal build process?

> +
> +define CRIO_INSTALL_TARGET_CMDS
> +	$(HOST_GO_COMMON_ENV) $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) \
> +		DESTDIR=$(TARGET_DIR) PREFIX=$(TARGET_DIR)/usr ETCDIR=$(TARGET_DIR)/etc \
> +		OPT_CNI_BIN_DIR=$(TARGET_DIR)/opt/cni/bin install.bin-nobuild

There's no proper installation step with the Go build infrastructure?

> +	$(INSTALL) -d -m 700 $(TARGET_DIR)/etc/cni
> +	$(INSTALL) -d -m 700 $(TARGET_DIR)/etc/cni/net.d

These two lines are not needed. Intermediate directories are created...

> +	$(INSTALL) -D -m 644 $(@D)/contrib/cni/10-crio-bridge.conflist \
> +		$(TARGET_DIR)/etc/cni/net.d/10-crio-bridge.conflist

... by the -D option here.

> +endef
> +
> +define CRIO_INSTALL_INIT_SYSTEMD
> +	$(HOST_GO_COMMON_ENV) $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) \
> +		DESTDIR=$(TARGET_DIR) PREFIX=$(TARGET_DIR)/usr ETCDIR=$(TARGET_DIR)/etc \
> +		OPT_CNI_BIN_DIR=$(TARGET_DIR)/opt/cni/bin install.systemd
> +	$(SED) 's,/usr/local/bin,/usr/bin,g' \
> +		$(TARGET_DIR)/usr/lib/systemd/system/{crio,crio-wipe}.service

Meh, the service files are incorrectly generated during the build? Or
are they not generated and simply contain values that need to manually
be tweaked?

Could you rework your patch to address the above suggestions?

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