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

[Henning Schild <henning.schild@siemens.com>]

Am Mon, 2 Aug 2021 15:27:58 +0200
schrieb Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>:

> On 02/08/2021 15:14, Henning Schild wrote:
> > Am Mon, 2 Aug 2021 14:26:02 +0200
> > schrieb 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?  
> > 
> > Oh right. Picked the wrong one.
> >   
> >>> +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 ;-)  
> > 
> > Ok ... at least you give some insights on the other comment. But you
> > started the "CONTAINER_" thingy. Can containers (not images) have
> > the format docker-archive ? ;)  
> 
> You're fully right. Shame on me :-/
> 
> Do as I say, not as I do ;-P
> 
> >   
> >> 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.  
> > 
> > Usage:  docker tag SOURCE_IMAGE[:TAG] TARGET_IMAGE[:TAG]
> > 
> > "CONTAINER_IMAGE_" because that is the name of the class ... and
> > pretty close to how other classes expose their "configurables"
> > except for the weird extension pattern you took over wrong from
> > that truly weird SDK.
> > 
> > Which gives me
> > 
> > CONTAINER_IMAGE_FORMATS  
> 
> I agree that for consistency you would have to rename this variable.
> Even if it breaks the interface, I think it's worth since this
> feature is not being widely used yet.
> 
> > CONTAINER_IMAGE_TAG
> > CONTAINER_IMAGE_TARGET_IMAGE  
> 
> Why CONTAINER_IMAGE_TARGET_IMAGE? Why not CONTAINER_IMAGE_NAME?
> "docker tag" needs the "TARGET_" prefix because it needs to
> differentiate it from the source image.
> 
> In fact if you look for the nowadays right manpage "man
> docker-image-tag" you'll get an even "better" mixture of
> "SOURCE_IMAGE", "TARGET_IMAGE" AND "NAME". But at least the
> terminology "NAME" and "TAG" appears here clearly defined.

Ok ... read this too late for v5. I will take NAME because it is used
in that docker manpage .. and it is somewhat appearing here

Usage:
  podman tag IMAGE TARGET_NAME [TARGET_NAME...]

So CONTAINER_IMAGE_NAME:CONTAINER_IMAGE_TAG ... before anyone goes and
looks it up in oci standards.

Henning

>   Silvano
> 
> > 
> > While the last one seems pretty silly ... i will still take it
> > because is is prefix+suffix following a clear pattern.
> > 
> > Henning
> >   
> >>   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
> >>>  }
> >>>  
> >>>     
> >