Re: [Buildroot] [PATCH v3 2/2] package/podman: new package

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

Hello Christian,

On Mon, 22 May 2023 13:23:46 -0700
Christian Stewart via buildroot <buildroot@buildroot.org> wrote:

> podman is a CLI tool for spawning and running containers according to the OCI
> specification.
> 
> https://github.com/containers/podman
> 
> Signed-off-by: Christian Stewart <christian@aperture.us>

Overall it looks good of course, but I have a couple of questions.

> diff --git a/package/podman/Config.in b/package/podman/Config.in
> new file mode 100644
> index 0000000000..92f5d6ae3d
> --- /dev/null
> +++ b/package/podman/Config.in
> @@ -0,0 +1,58 @@
> +config BR2_PACKAGE_PODMAN
> +	bool "podman"
> +	depends on !BR2_TOOLCHAIN_USES_UCLIBC # no fexecve

I don't think that's true. That was inherited from crun, and I just
sent a patch that allows crun to build with uClibc.

> +	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 # conmon -> libglib2
> +	depends on BR2_USE_MMU # libgpgme, conmon -> libglib2
> +	depends on BR2_USE_WCHAR # conmon -> libglib2

I think you can just say "conmon, libglib2" as you inherit the
dependencies from both.

> +	select BR2_PACKAGE_CGROUPFS_MOUNT if !BR2_PACKAGE_SYSTEMD # runtime
> +	select BR2_PACKAGE_CNI_PLUGINS
> +	select BR2_PACKAGE_CONMON
> +	select BR2_PACKAGE_CRUN
> +	select BR2_PACKAGE_IPTABLES
> +	select BR2_PACKAGE_LIBGLIB2
> +	select BR2_PACKAGE_LIBGPGME

Out of all those dependencies, you only have build dependencies on
libglib2 and libgpgme. Are the other runtime dependencies? If so,
please add a # runtime comment, like you did for the cgroupfs-mount
dependency.


> +if BR2_PACKAGE_PODMAN
> +
> +config BR2_PACKAGE_PODMAN_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 BR2_PACKAGE_PODMAN_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.
> +
> +config BR2_PACKAGE_PODMAN_DRIVER_VFS
> +	bool "vfs filesystem driver"
> +	depends on BR2_USE_WCHAR # gvfs
> +	depends on BR2_USE_MMU # gvfs
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # gvfs
> +	depends on !BR2_STATIC_LIBS # gvfs
> +	select BR2_PACKAGE_GVFS
> +	help
> +	  Build the vfs filesystem driver.

Normally, we would need a Config.in comment about the dependencies, but
I agree almost all of them are covered by the top-level option, and the
only one remaining is !BR2_STATIC_LIBS... Perhaps not worth adding a
Config.in comment for this, I agree.

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

Needs to be adjusted once the uClibc exclusion is removed.

> diff --git a/package/podman/podman.mk b/package/podman/podman.mk
> new file mode 100644
> index 0000000000..f888f3ae5e
> --- /dev/null
> +++ b/package/podman/podman.mk
> @@ -0,0 +1,75 @@
> +################################################################################
> +#
> +# podman
> +#
> +################################################################################
> +
> +PODMAN_VERSION = 4.5.0
> +PODMAN_SITE = $(call github,containers,podman,v$(PODMAN_VERSION))
> +PODMAN_LICENSE = Apache-2.0
> +PODMAN_LICENSE_FILES = LICENSE
> +
> +PODMAN_CPE_ID_VENDOR = podman_project
> +PODMAN_GOMOD = github.com/containers/podman/v4
> +
> +PODMAN_BUILD_TARGETS = cmd/podman cmd/rootlessport cmd/quadlet
> +PODMAN_DEPENDENCIES += libglib2 libgpgme

Change += to =.


> +define PODMAN_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr \
> +		install.bin
> +	$(INSTALL) -d -m 700 $(TARGET_DIR)/etc/cni
> +	$(INSTALL) -d -m 700 $(TARGET_DIR)/etc/cni/net.d

What do you need this?

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

$(INSTALL) -D will create all directories that are needed to allow
creating the destination file.

> +	$(INSTALL) -d -m 755 $(TARGET_DIR)/etc/containers
> +	$(INSTALL) -D -m 644 $(PODMAN_PKGDIR)/containers-policy.json \
> +		$(TARGET_DIR)/etc/containers/policy.json

Same comment.

> +endef
> +
> +define PODMAN_INSTALL_INIT_SYSTEMD
> +	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) PREFIX=/usr \
> +		install.systemd
> +endef
> +
> +$(eval $(golang-package))

Otherwise, looks good. So it's really the question about build-time vs.
run-time dependencies that didn't allow me to tweak the patch when
applying.

Could you rework the patch and send a new iteration? I promise, it will
take less than 2 months to be handled :-)

Thanks!

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