| Message ID | 20220127062504.1835450-2-christian@paral.in |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3,1/2] package/runc: add host package | expand |
Hello, On Wed, 26 Jan 2022 22:25:04 -0800 Christian Stewart <christian@paral.in> wrote: > v1 -> v2: > > - add package to developers > - add host runc dependency for host package > - add libgpgme runtime dependency Considering that there are runtime dependency concerns, it would be nice to have a simple test case in support/testing/. > diff --git a/package/buildah/buildah.hash b/package/buildah/buildah.hash > new file mode 100644 > index 0000000000..c7e00d02a7 > --- /dev/null > +++ b/package/buildah/buildah.hash > @@ -0,0 +1,3 @@ > +# Locally calculated > +sha256 d99b5187a25bc9d7385408732a0e155df0458b4d2cea6e8d002f3fa2cbaac76f buildah-1.24.0.tar.gz > +sha256 b40930bbcf80744c86c46a12bc9da056641d722716c378f5659b9e555ef833e1 LICENSE > diff --git a/package/buildah/buildah.mk b/package/buildah/buildah.mk > new file mode 100644 > index 0000000000..658d7ef56f > --- /dev/null > +++ b/package/buildah/buildah.mk > @@ -0,0 +1,46 @@ > +################################################################################ > +# > +# buildah > +# > +################################################################################ > + > +BUILDAH_VERSION = 1.24.0 > +BUILDAH_SITE = $(call github,containers,buildah,v$(BUILDAH_VERSION)) > + > +BUILDAH_LICENSE = Apache-2.0 > +BUILDAH_LICENSE_FILES = LICENSE > + > +BUILDAH_DEPENDENCIES = libgpgme Is libgpgme really a runtime dependency, as noted in your changelog? If it is, then it's not needed in BUILDAH_DEPENDENCIES. However, it would be somewhat surprising for it to be only a runtime dependency. Could you confirm? Does it get dlopen()ed at runtime? Or it's not the library that is used at runtime, but some program that is installed by libgpgme? > +define BUILDAH_INSTALL_CONFIG > + $(INSTALL) -D -m 644 $(@D)/contrib/buildroot/policy.json \ > + $(TARGET_DIR)/etc/containers/policy.json > +endef > + > +BUILDAH_POST_INSTALL_TARGET_HOOKS += BUILDAH_INSTALL_CONFIG > + > +HOST_BUILDAH_BUILD_TARGETS = $(BUILDAH_BUILD_TARGETS) > +HOST_BUILDAH_TAGS = $(BUILDAH_TAGS) > +HOST_BUILDAH_LDFLAGS = $(BUILDAH_LDFLAGS) > +HOST_BUILDAH_INSTALL_BINS = $(BUILDAH_INSTALL_BINS) That "repetition" also makes me think we should have some level of inheritance between target and host variables in the golang-package infrastructure. For BUILD_TARGETS, TAGS and INSTALL_BINS, it sounds fine to do it. However, for LDFLAGS, it's a bit weird, as normally, LDFLAGS are different between host and target. However here, there are mostly used to pass these version-related -X options, that are in fact the same between host and target. Should we have a separate variable to pass those flags ? Not sure. Thomas
Hi Thomas, On Thu, Jan 27, 2022 at 12:01 AM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Considering that there are runtime dependency concerns, it would be > nice to have a simple test case in support/testing/. OK, something like "buildah pull" or so? > > +BUILDAH_DEPENDENCIES = libgpgme > > Is libgpgme really a runtime dependency, as noted in your changelog? If Gpg is a runtime dependency, libgpgme is a build dependency. > > +HOST_BUILDAH_BUILD_TARGETS = $(BUILDAH_BUILD_TARGETS) > > +HOST_BUILDAH_TAGS = $(BUILDAH_TAGS) > > +HOST_BUILDAH_LDFLAGS = $(BUILDAH_LDFLAGS) > > +HOST_BUILDAH_INSTALL_BINS = $(BUILDAH_INSTALL_BINS) > > That "repetition" also makes me think we should have some level of > inheritance between target and host variables in the golang-package > infrastructure. For BUILD_TARGETS, TAGS and INSTALL_BINS, it sounds > fine to do it. Yes - unless the package overrides them - this should be fine. I was a bit surprised that it, on default, tries to build the binary named "host-runc" as well as a Go package named "host-runc". Of course, this is not the name of neither the binary nor the package. So the host-golang infra should remove the host- prefix for the default BUILD_TARGETS and INSTALL_BINS. This would also be fixed if those values were inherited from the target package. > However, for LDFLAGS, it's a bit weird, as normally, > LDFLAGS are different between host and target. However here, there are > mostly used to pass these version-related -X options, that are in fact > the same between host and target. Should we have a separate variable to > pass those flags ? Not sure. They are quite common for Go packages - could call them DEFINES or something similar. The syntax overrides the value of a global variable in a package. Best regards, Christian
Christian, All, Sorry for the late review... (I've also quoted snippets from your commit log) On 2022-01-26 12:25 -0800, Christian Stewart via buildroot spake thusly: > Adds both host and target packages for buildah. I would question the need for having buildah on the target. Buildah is advertised as: "A tool that facilitates building OCI container images." I don't see a good reason for building images *on* the target. Please explain in the commit log why that would make sense to have buildah on the target. > The buildah tree does not ship with a default policy.json file, and instead > relies on packagers to provide one. A patch is added to create a basic barebones > policy.json which is installed to /etc/containers/policy.json with a hook. Don't add a patch; just add that as a file in the Buildroot tree, e.g. package/buildah/policy.json, and install that from the package dir, i.e $(BUILDAH_PKGDIR)/policy.json rather than from $(@D)/..../policy.json. On 2022-01-27 12:31 -0800, Christian Stewart via buildroot spake thusly: > On Thu, Jan 27, 2022 at 12:01 AM Thomas Petazzoni > <thomas.petazzoni@bootlin.com> wrote: > > Considering that there are runtime dependency concerns, it would be > > nice to have a simple test case in support/testing/. > OK, something like "buildah pull" or so? A runtime test should validate that the package works as expected, in a minimal way. So, if 'buildah pull' can prove that runtime dependencies are exercised, then that's OK, yes. Obviously if runtiem dependencies are only required for specific cases and only "loaded" when needed, we will not notice any missing ones. But having an exhaustive test is not very important either; we do not want to have to run the full project's test-suite... [--SNIP--] > I was a bit surprised that it, on default, tries to build the binary > named "host-runc" as well as a Go package named "host-runc". Of > course, this is not the name of neither the binary nor the package. > So the host-golang infra should remove the host- prefix for the > default BUILD_TARGETS and INSTALL_BINS. We now have 8539378771f7 (package/pkg-golang: default to rawname to install binaries) which is supopsed to fix that. > This would also be fixed if those values were inherited from the target package. I am very wary of inheriting target values for host packages. Back in the day, we had such an inheritance in the autotools infra, but we eventually got rid of it because it was causing too much issue, see 4bdb067e380e (infra: remove auto derivation of host dependencies). The only variables that should be automatically inherited are those that actualy identify the package, like the site, the version, the license, etc.. and not the ones that /define/ it (configure and build options and so on...) Here, TAGS and BUILD_TARGETS are typically not something that should be automatically inherited, because they define what is built, and the host and target packages should not by default have the same feature set (we tend to only have conditionals for the target one, and enable everything in the host variant, for example). The GOMOD you provided suspiciously looks like what the infra would have computed already, no? > > However, for LDFLAGS, it's a bit weird, as normally, > > LDFLAGS are different between host and target. However here, there are > > mostly used to pass these version-related -X options, that are in fact > > the same between host and target. Should we have a separate variable to > > pass those flags ? Not sure. > They are quite common for Go packages - could call them DEFINES or > something similar. But with LDFLAGS, it is possible to pass flags to the actual linker, with -extldflags something-something, so they should not be automatically inherited. And if we had something the DEFINES you hint at, they would again be different by default, because the host and target builds do not follow the same logic. I've marked this patch, the oldest in patchwork, as changes-requested. Regards, Yann E. MORIN. > The syntax overrides the value of a global variable in a package. > > Best regards, > Christian > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Yann, On Sun, Aug 21, 2022, 8:14 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > On 2022-01-26 12:25 -0800, Christian Stewart via buildroot spake thusly: > > Adds both host and target packages for buildah. > > I would question the need for having buildah on the target. Buildah is > advertised as: "A tool that facilitates building OCI container images." > I don't see a good reason for building images *on* the target. Please > explain in the commit log why that would make sense to have buildah on > the target. > Building container images with a tool like "docker build" on the target is an extremely common practice. There are numerous reasons to do so. Almost none of the container images I use on devices were built ahead of time, rather on the target device. I don't see why it's necessary to explain that on every commit message. > The buildah tree does not ship with a default policy.json file, and > instead > > relies on packagers to provide one. A patch is added to create a basic > barebones > > policy.json which is installed to /etc/containers/policy.json with a > hook. > > Don't add a patch; just add that as a file in the Buildroot tree, e.g. > package/buildah/policy.json, and install that from the package dir, i.e > $(BUILDAH_PKGDIR)/policy.json rather than from $(@D)/..../policy.json. > Did a similar thing to this with the upcoming podman package. Will update. On 2022-01-27 12:31 -0800, Christian Stewart via buildroot spake thusly: > > On Thu, Jan 27, 2022 at 12:01 AM Thomas Petazzoni > > <thomas.petazzoni@bootlin.com> wrote: > > > Considering that there are runtime dependency concerns, it would be > > > nice to have a simple test case in support/testing/. > > OK, something like "buildah pull" or so? > > A runtime test should validate that the package works as expected, in a > minimal way. So, if 'buildah pull' can prove that runtime dependencies > are exercised, then that's OK, yes. > > Obviously if runtiem dependencies are only required for specific cases > and only "loaded" when needed, we will not notice any missing ones. But > having an exhaustive test is not very important either; we do not want > to have to run the full project's test-suite... > I would argue that in this case if the package compiles at all it will work, as per the upstream tests. Not sure if it's really worth the time to add the extra test here. > > However, for LDFLAGS, it's a bit weird, as normally, > > > LDFLAGS are different between host and target. However here, there are > > > mostly used to pass these version-related -X options, that are in fact > > > the same between host and target. Should we have a separate variable to > > > pass those flags ? Not sure. > > They are quite common for Go packages - could call them DEFINES or > > something similar. > > But with LDFLAGS, it is possible to pass flags to the actual linker, > with -extldflags something-something, so they should not be > automatically inherited. > > And if we had something the DEFINES you hint at, they would again be > different by default, because the host and target builds do not follow > the same logic. > We are defining the Version here, which is the same on the host and target. Thanks, Christian
diff --git a/DEVELOPERS b/DEVELOPERS index fe8de1916e..80e7c5abee 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -529,6 +529,7 @@ F: package/python-pylibftdi/ N: Christian Stewart <christian@paral.in> F: package/batman-adv/ +F: package/buildah/ F: package/containerd/ F: package/delve/ F: package/docker-cli/ diff --git a/package/Config.in b/package/Config.in index e4ca195beb..91eb66a454 100644 --- a/package/Config.in +++ b/package/Config.in @@ -2523,6 +2523,7 @@ menu "System tools" source "package/audit/Config.in" source "package/balena-engine/Config.in" source "package/bubblewrap/Config.in" + source "package/buildah/Config.in" source "package/cgroupfs-mount/Config.in" source "package/circus/Config.in" source "package/containerd/Config.in" diff --git a/package/Config.in.host b/package/Config.in.host index 2ce015eacf..6aadb1b640 100644 --- a/package/Config.in.host +++ b/package/Config.in.host @@ -7,6 +7,7 @@ menu "Host utilities" source "package/babeltrace2/Config.in.host" source "package/bmap-tools/Config.in.host" source "package/btrfs-progs/Config.in.host" + source "package/buildah/Config.in.host" source "package/cbootimage/Config.in.host" source "package/checkpolicy/Config.in.host" source "package/checksec/Config.in.host" diff --git a/package/buildah/0001-contrib-add-buildroot-policy-json.patch b/package/buildah/0001-contrib-add-buildroot-policy-json.patch new file mode 100644 index 0000000000..7a8ca57a5e --- /dev/null +++ b/package/buildah/0001-contrib-add-buildroot-policy-json.patch @@ -0,0 +1,38 @@ +From 6808cfa788f03fca36a41202d9475ee5bc9feac7 Mon Sep 17 00:00:00 2001 +From: Christian Stewart <christian@paral.in> +Date: Wed, 26 Jan 2022 22:07:09 -0800 +Subject: [PATCH] contrib: add buildroot policy json + +Buildah does not ship a default policy.json in-tree. + +Signed-off-by: Christian Stewart <christian@paral.in> +--- + contrib/buildroot/policy.json | 16 ++++++++++++++++ + 1 file changed, 16 insertions(+) + create mode 100644 contrib/buildroot/policy.json + +diff --git a/contrib/buildroot/policy.json b/contrib/buildroot/policy.json +new file mode 100644 +index 00000000..d8c638a0 +--- /dev/null ++++ b/contrib/buildroot/policy.json +@@ -0,0 +1,16 @@ ++{ ++ "default": [ ++ { ++ "type": "insecureAcceptAnything" ++ } ++ ], ++ "transports": { ++ "docker-daemon": { ++ "": [ ++ { ++ "type": "insecureAcceptAnything" ++ } ++ ] ++ } ++ } ++} +-- +2.35.0 + diff --git a/package/buildah/Config.in b/package/buildah/Config.in new file mode 100644 index 0000000000..05bd0eec31 --- /dev/null +++ b/package/buildah/Config.in @@ -0,0 +1,23 @@ +config BR2_PACKAGE_BUILDAH + bool "buildah" + depends on BR2_PACKAGE_HOST_GO_TARGET_ARCH_SUPPORTS + depends on BR2_PACKAGE_HOST_GO_TARGET_CGO_LINKING_SUPPORTS + depends on BR2_PACKAGE_LIBGPG_ERROR_ARCH_SUPPORTS # libgpgme -> libgpg-error + depends on BR2_TOOLCHAIN_HAS_THREADS # runc + depends on BR2_USE_MMU # libgpgme -> libassuan + depends on !BR2_TOOLCHAIN_USES_UCLIBC # runc -> no fexecve + # gnupg and runc are not needed to build, but at runtime. + select BR2_PACKAGE_LIBGPGME + select BR2_PACKAGE_GNUPG if !BR2_PACKAGE_GNUPG2 + select BR2_PACKAGE_LIBGPG_ERROR + select BR2_PACKAGE_LIBASSUAN + select BR2_PACKAGE_RUNC + help + Buildah is a tool that facilitates building OCI images. + + https://github.com/containers/buildah + +comment "buildah 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 diff --git a/package/buildah/Config.in.host b/package/buildah/Config.in.host new file mode 100644 index 0000000000..67fee6d7ac --- /dev/null +++ b/package/buildah/Config.in.host @@ -0,0 +1,8 @@ +config BR2_PACKAGE_HOST_BUILDAH + bool "host buildah" + depends on BR2_PACKAGE_HOST_GO_HOST_ARCH_SUPPORTS + select BR2_PACKAGE_HOST_RUNC + help + Buildah is a tool that facilitates building OCI images. + + https://github.com/containers/buildah diff --git a/package/buildah/buildah.hash b/package/buildah/buildah.hash new file mode 100644 index 0000000000..c7e00d02a7 --- /dev/null +++ b/package/buildah/buildah.hash @@ -0,0 +1,3 @@ +# Locally calculated +sha256 d99b5187a25bc9d7385408732a0e155df0458b4d2cea6e8d002f3fa2cbaac76f buildah-1.24.0.tar.gz +sha256 b40930bbcf80744c86c46a12bc9da056641d722716c378f5659b9e555ef833e1 LICENSE diff --git a/package/buildah/buildah.mk b/package/buildah/buildah.mk new file mode 100644 index 0000000000..658d7ef56f --- /dev/null +++ b/package/buildah/buildah.mk @@ -0,0 +1,46 @@ +################################################################################ +# +# buildah +# +################################################################################ + +BUILDAH_VERSION = 1.24.0 +BUILDAH_SITE = $(call github,containers,buildah,v$(BUILDAH_VERSION)) + +BUILDAH_LICENSE = Apache-2.0 +BUILDAH_LICENSE_FILES = LICENSE + +BUILDAH_DEPENDENCIES = libgpgme + +BUILDAH_CPE_ID_VENDOR = buildah_project +BUILDAH_CPE_ID_PRODUCT = buildah + +BUILDAH_TAGS = \ + cgo \ + exclude_graphdriver_aufs \ + exclude_graphdriver_btrfs \ + exclude_graphdriver_devicemapper \ + exclude_graphdriver_zfs +BUILDAH_BUILD_TARGETS = cmd/buildah +BUILDAH_GOMOD = github.com/containers/buildah + +BUILDAH_LDFLAGS = \ + -X $(BUILDAH_GOMOD)/cmd/buildah.GitCommit=v$(BUILDAH_VERSION) \ + -X $(BUILDAH_GOMOD)/define.Version=v$(BUILDAH_VERSION) + +BUILDAH_INSTALL_BINS = $(notdir $(BUILDAH_BUILD_TARGETS)) + +define BUILDAH_INSTALL_CONFIG + $(INSTALL) -D -m 644 $(@D)/contrib/buildroot/policy.json \ + $(TARGET_DIR)/etc/containers/policy.json +endef + +BUILDAH_POST_INSTALL_TARGET_HOOKS += BUILDAH_INSTALL_CONFIG + +HOST_BUILDAH_BUILD_TARGETS = $(BUILDAH_BUILD_TARGETS) +HOST_BUILDAH_TAGS = $(BUILDAH_TAGS) +HOST_BUILDAH_LDFLAGS = $(BUILDAH_LDFLAGS) +HOST_BUILDAH_INSTALL_BINS = $(BUILDAH_INSTALL_BINS) + +$(eval $(golang-package)) +$(eval $(host-golang-package))