Re: [PATCH v3 3/4] classes: allow more control over container image name and tag

[Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>]

On 02/08/2021 13:54, Henning Schild wrote:
> This patch allows more fine-grained control over how the resulting
> container will be tagged. Where the default name will be PN together
> with DISTRO and ARCH, and tag will be derived from PV and PR
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  RECIPE-API-CHANGELOG.md                        |  4 ++++
>  doc/user_manual.md                             |  8 ++++----
>  meta/classes/container-img.bbclass             |  4 +---
>  meta/classes/image-container-extension.bbclass | 11 ++++++-----
>  meta/classes/image-sdk-extension.bbclass       |  2 +-
>  5 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> index 806c2914f13f..d12d30e192a8 100644
> --- a/RECIPE-API-CHANGELOG.md
> +++ b/RECIPE-API-CHANGELOG.md
> @@ -292,3 +292,7 @@ Migrate your patches so they can be applied with "git am", or
>  
>  Kernel image name for arm64 platforms is vmlinux now. Image format was
>  not changed (uncompressed executable) but now it is named correctly.
> +
> +### Change default "TARGET_NAME:TAG" when building container images
> +
> +The "TARGET_NAME" used to be rather static and the TAG was always "latest", now the values are derived from recipe variables PN, PV, PR.
> diff --git a/doc/user_manual.md b/doc/user_manual.md
> index cf7dc2fee35e..e5c07bd41fc5 100644
> --- a/doc/user_manual.md
> +++ b/doc/user_manual.md
> @@ -300,14 +300,14 @@ bitbake mc:qemuarm-buster:isar-image-base
>   - Load the container image into the Docker Daemon
>  
>  ```
> -docker load -i build/tmp/deploy/images/qemuarm/debian-buster-armhf-docker-archive.tar.xz
> +docker load -i build/tmp/deploy/images/qemuarm/isar-image-base-debian-buster-armhf-1.0-r0-docker-archive.tar.xz
>  ```
>  
>   - Run a container using the container image (following commands starting with 
>     `#~:` are to be run in the container)
>  
>  ```
> -docker run --rm -ti --volume "$(pwd):/build" isar-buster-armhf:latest
> +docker run --rm -ti --volume "$(pwd):/build" isar-image-base-debian-buster-armhf:1.0-r0
>  ```
>  
>  ---
> @@ -1070,14 +1070,14 @@ bitbake -c do_populate_sdk mc:qemuarm-buster:isar-image-base
>   - Load the SDK container image into the Docker Daemon
>  
>  ```
> -docker load -i build/tmp/deploy/images/qemuarm/sdk-debian-buster-armhf-docker-archive.tar.xz
> +docker load -i build/tmp/deploy/images/qemuarm/isar-image-base-debian-buster-armhf-1.0-r0-docker-archive.tar.xz
>  ```
>  
>   - Run a container using the SDK container image (following commands starting 
>     with `#~:` are to be run in the container)
>  
>  ```
> -docker run --rm -ti --volume "$(pwd):/build" isar-sdk-buster-armhf:latest
> +docker run --rm -ti --volume "$(pwd):/build" isar-image-base-debian-buster-armhf:1.0-r0
>  ```
>  
>   - Check that cross toolchains are installed
> diff --git a/meta/classes/container-img.bbclass b/meta/classes/container-img.bbclass
> index 79ef3e8d756b..9928a58ef53d 100644
> --- a/meta/classes/container-img.bbclass
> +++ b/meta/classes/container-img.bbclass
> @@ -9,10 +9,8 @@
>  do_container_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
>  do_container_image[vardeps] += "CONTAINER_FORMATS"
>  do_container_image(){
> -    rootfs_id="${DISTRO}-${DISTRO_ARCH}"
> -
>      bbdebug 1 "Generate container image in these formats: ${CONTAINER_FORMATS}"
> -    containerize_rootfs "${IMAGE_ROOTFS}" "${rootfs_id}" "${CONTAINER_FORMATS}"
> +    containerize_rootfs "${IMAGE_ROOTFS}" "${CONTAINER_FORMATS}"
>  }
>  
>  addtask container_image before do_image after do_image_tools
> diff --git a/meta/classes/image-container-extension.bbclass b/meta/classes/image-container-extension.bbclass
> index 0e70ba9c1405..4f93d30219f9 100644
> --- a/meta/classes/image-container-extension.bbclass
> +++ b/meta/classes/image-container-extension.bbclass
> @@ -6,15 +6,16 @@
>  # This class extends the image.bbclass for containerizing the root filesystem.
>  
>  CONTAINER_FORMATS ?= "docker-archive"
> +CONTAINER_TARGET_NAME ?= "${PN}-${DISTRO}-${DISTRO_ARCH}"

In a comment to patch v2 you mentioned your plan to call it "CONTAINER_TARGET_IMAGE".
Have you changed your mind or is it just an error?

> +CONTAINER_TAG ?= "${PV}-${PR}"

Although only container images can have tags and not containers, I still find this name misleading for container newbies (and some ISAR maintainers will probably be).
Your argument that "docker tag" only uses "TAG" doesn't convince me ;-)

In general I'd say that both "CONTAINER_TARGET_NAME" and "CONTAINER_TAG" should start with the prefix "CONTAINER_IMAGE_".
Both the name and the tag are attributes of a container image.

Therefore I'd rather use "CONTAINER_IMAGE_NAME" and "CONTAINER_IMAGE_TAG".
But since I'm neither a power user nor a power contributor, my opinion might not be that important.

  Silvano

>  
>  containerize_rootfs() {
>      local cmd="/bin/dash"
>      local empty_tag="empty"
> -    local tag="latest"
> +    local tag="${CONTAINER_TAG}"
>      local oci_img_dir="${WORKDIR}/oci-image"
>      local rootfs="$1"
> -    local rootfs_id="$2"
> -    local container_formats="$3"
> +    local container_formats="$2"
>  
>      # prepare OCI container image skeleton
>      bbdebug 1 "prepare OCI container image skeleton"
> @@ -42,9 +43,9 @@ containerize_rootfs() {
>      sudo chown --recursive $(id -u):$(id -g) "${oci_img_dir}"
>  
>      # convert the OCI container image to the desired format
> -    image_name="isar-${rootfs_id}"
> +    image_name="${CONTAINER_TARGET_NAME}"
>      for image_type in ${CONTAINER_FORMATS} ; do
> -        image_archive="${DEPLOY_DIR_IMAGE}/${rootfs_id}-${image_type}.tar"
> +        image_archive="${DEPLOY_DIR_IMAGE}/${image_name}-${tag}-${image_type}.tar"
>          bbdebug 1 "Creating container image type: ${image_type}"
>          case "${image_type}" in
>              "docker-archive" | "oci-archive")
> diff --git a/meta/classes/image-sdk-extension.bbclass b/meta/classes/image-sdk-extension.bbclass
> index 426b92595554..fa15b588068c 100644
> --- a/meta/classes/image-sdk-extension.bbclass
> +++ b/meta/classes/image-sdk-extension.bbclass
> @@ -80,7 +80,7 @@ do_populate_sdk() {
>      # generate the SDK in all the desired container formats
>      if [ -n "${sdk_container_formats}" ] ; then
>          bbnote "Generating SDK container in ${sdk_container_formats} format"
> -        containerize_rootfs "${SDKCHROOT_DIR}" "sdk-${DISTRO}-${DISTRO_ARCH}" "${sdk_container_formats}"
> +        containerize_rootfs "${SDKCHROOT_DIR}" "${sdk_container_formats}"
>      fi
>  }
>  
> 
-- 
  Silvano Cirujano Cuesta
-- 
Siemens AG, T RDA IOT SES-DE
Corporate Competence Center Embedded Linux