[PATCH 0/4] Allow better control over container tags

[PATCH 0/4] Allow better control over container tags

[Henning Schild]

A container tags consists of "name:tag" and both have been kind of
hardcoded so far. "tag" was always "latest" which is not proper
versioning. And "name" has always been "isar-DISTRO-ARCH" which is also
not proper because the name should probably describe the content and not
so much the build system. Plus that name means guaranteed collision if
you build two containers with isar based on the same disto and arch.

p3 is kind of interface breaking, i may have forgotten some docs and
stuff

Henning Schild (4):
  classes: make sure container extension can run multiple times
  classes: simplify tag handling in container class
  classes: allow more control over container image name and tag
  classes: fix comment in container-img class

 meta/classes/container-img.bbclass            |  6 ++---
 .../classes/image-container-extension.bbclass | 25 ++++++++++---------
 meta/classes/image-sdk-extension.bbclass      |  2 +-
 3 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH 1/4] classes: make sure container extension can run multiple times

[Henning Schild]

If the container extension fails somewhere in the middle, it could leave
things behind resulting in issues when trying to run only that one task
again.

So make sure to remove stuff as root and remove another possibly
existing directory.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 meta/classes/image-container-extension.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/image-container-extension.bbclass b/meta/classes/image-container-extension.bbclass
index e26604a04f06..6537fc693265 100644
--- a/meta/classes/image-container-extension.bbclass
+++ b/meta/classes/image-container-extension.bbclass
@@ -18,7 +18,7 @@ containerize_rootfs() {
 
     # prepare OCI container image skeleton
     bbdebug 1 "prepare OCI container image skeleton"
-    rm -rf "${oci_img_dir}"
+    sudo rm -rf "${oci_img_dir}" "${oci_img_dir}_unpacked"
     sudo umoci init --layout "${oci_img_dir}"
     sudo umoci new --image "${oci_img_dir}:${empty_tag}"
     sudo umoci config --image "${oci_img_dir}:${empty_tag}" \
-- 
2.31.1

Re: [PATCH 1/4] classes: make sure container extension can run multiple times

[Silvano Cirujano Cuesta]

On 28/07/2021 17:22, Henning Schild wrote:
> If the container extension fails somewhere in the middle, it could leave
> things behind resulting in issues when trying to run only that one task
> again.
>
> So make sure to remove stuff as root and remove another possibly
> existing directory.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  meta/classes/image-container-extension.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meta/classes/image-container-extension.bbclass b/meta/classes/image-container-extension.bbclass
> index e26604a04f06..6537fc693265 100644
> --- a/meta/classes/image-container-extension.bbclass
> +++ b/meta/classes/image-container-extension.bbclass
> @@ -18,7 +18,7 @@ containerize_rootfs() {
>  
>      # prepare OCI container image skeleton
>      bbdebug 1 "prepare OCI container image skeleton"
> -    rm -rf "${oci_img_dir}"
> +    sudo rm -rf "${oci_img_dir}" "${oci_img_dir}_unpacked"
Correct, since those directory trees are beings created with 'sudo'.
>      sudo umoci init --layout "${oci_img_dir}"
>      sudo umoci new --image "${oci_img_dir}:${empty_tag}"
>      sudo umoci config --image "${oci_img_dir}:${empty_tag}" \

  Silvano Cirujano Cuesta
-- 
Siemens AG, T RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[PATCH 2/4] classes: simplify tag handling in container class

[Henning Schild]

Call the variable "tag" and use it in all places where "latest" was
hardcoded. This prepares for another patch that will allow chosing
something else than "latest" for a tag.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 meta/classes/image-container-extension.bbclass | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/meta/classes/image-container-extension.bbclass b/meta/classes/image-container-extension.bbclass
index 6537fc693265..0e70ba9c1405 100644
--- a/meta/classes/image-container-extension.bbclass
+++ b/meta/classes/image-container-extension.bbclass
@@ -10,7 +10,7 @@ CONTAINER_FORMATS ?= "docker-archive"
 containerize_rootfs() {
     local cmd="/bin/dash"
     local empty_tag="empty"
-    local full_tag="latest"
+    local tag="latest"
     local oci_img_dir="${WORKDIR}/oci-image"
     local rootfs="$1"
     local rootfs_id="$2"
@@ -33,7 +33,7 @@ containerize_rootfs() {
 
     # pack container image
     bbdebug 1 "pack container image"
-    sudo umoci repack --image "${oci_img_dir}:${full_tag}" \
+    sudo umoci repack --image "${oci_img_dir}:${tag}" \
         "${oci_img_dir}_unpacked"
     sudo umoci remove --image "${oci_img_dir}:${empty_tag}"
     sudo rm -rf "${oci_img_dir}_unpacked"
@@ -49,14 +49,14 @@ containerize_rootfs() {
         case "${image_type}" in
             "docker-archive" | "oci-archive")
                 if [ "${image_type}" = "oci-archive" ] ; then
-                    target="${image_type}:${image_archive}:latest"
+                    target="${image_type}:${image_archive}:${tag}"
                 else
-                    target="${image_type}:${image_archive}:${image_name}:latest"
+                    target="${image_type}:${image_archive}:${image_name}:${tag}"
                 fi
                 rm -f "${image_archive}" "${image_archive}.xz"
                 bbdebug 2 "Converting OCI image to ${image_type}"
                 skopeo --insecure-policy copy \
-                    "oci:${oci_img_dir}:${full_tag}" "${target}"
+                    "oci:${oci_img_dir}:${tag}" "${target}"
                 bbdebug 2 "Compressing image"
                 xz -T0 "${image_archive}"
                 ;;
@@ -69,8 +69,8 @@ containerize_rootfs() {
                     die "Adding the container image to a container runtime (${image_type}) not supported if running from a container (e.g. 'kas-container')"
                 fi
                 skopeo --insecure-policy copy \
-                    "oci:${oci_img_dir}:${full_tag}" \
-                    "${image_type}:${image_name}:latest"
+                    "oci:${oci_img_dir}:${tag}" \
+                    "${image_type}:${image_name}:${tag}"
                 ;;
             *)
                 die "Unsupported format for containerize_rootfs: ${image_type}"
-- 
2.31.1

Re: [PATCH 2/4] classes: simplify tag handling in container class

[Silvano Cirujano Cuesta]

On 28/07/2021 17:22, Henning Schild wrote:
> Call the variable "tag" and use it in all places where "latest" was
> hardcoded. This prepares for another patch that will allow chosing
> something else than "latest" for a tag.

I named it "full_tag" to remark the difference with the tag of the empty container image. But you're probably right, that simply "tag" is a better name.

Replacing the hard-coded tag is also meaningful.

>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  meta/classes/image-container-extension.bbclass | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/meta/classes/image-container-extension.bbclass b/meta/classes/image-container-extension.bbclass
> index 6537fc693265..0e70ba9c1405 100644
> --- a/meta/classes/image-container-extension.bbclass
> +++ b/meta/classes/image-container-extension.bbclass
> @@ -10,7 +10,7 @@ CONTAINER_FORMATS ?= "docker-archive"
>  containerize_rootfs() {
>      local cmd="/bin/dash"
>      local empty_tag="empty"
> -    local full_tag="latest"
> +    local tag="latest"
>      local oci_img_dir="${WORKDIR}/oci-image"
>      local rootfs="$1"
>      local rootfs_id="$2"
> @@ -33,7 +33,7 @@ containerize_rootfs() {
>  
>      # pack container image
>      bbdebug 1 "pack container image"
> -    sudo umoci repack --image "${oci_img_dir}:${full_tag}" \
> +    sudo umoci repack --image "${oci_img_dir}:${tag}" \
>          "${oci_img_dir}_unpacked"
>      sudo umoci remove --image "${oci_img_dir}:${empty_tag}"
>      sudo rm -rf "${oci_img_dir}_unpacked"
> @@ -49,14 +49,14 @@ containerize_rootfs() {
>          case "${image_type}" in
>              "docker-archive" | "oci-archive")
>                  if [ "${image_type}" = "oci-archive" ] ; then
> -                    target="${image_type}:${image_archive}:latest"
> +                    target="${image_type}:${image_archive}:${tag}"
>                  else
> -                    target="${image_type}:${image_archive}:${image_name}:latest"
> +                    target="${image_type}:${image_archive}:${image_name}:${tag}"
>                  fi
>                  rm -f "${image_archive}" "${image_archive}.xz"
>                  bbdebug 2 "Converting OCI image to ${image_type}"
>                  skopeo --insecure-policy copy \
> -                    "oci:${oci_img_dir}:${full_tag}" "${target}"
> +                    "oci:${oci_img_dir}:${tag}" "${target}"
>                  bbdebug 2 "Compressing image"
>                  xz -T0 "${image_archive}"
>                  ;;
> @@ -69,8 +69,8 @@ containerize_rootfs() {
>                      die "Adding the container image to a container runtime (${image_type}) not supported if running from a container (e.g. 'kas-container')"
>                  fi
>                  skopeo --insecure-policy copy \
> -                    "oci:${oci_img_dir}:${full_tag}" \
> -                    "${image_type}:${image_name}:latest"
> +                    "oci:${oci_img_dir}:${tag}" \
> +                    "${image_type}:${image_name}:${tag}"
>                  ;;
>              *)
>                  die "Unsupported format for containerize_rootfs: ${image_type}"

  Silvano Cirujano Cuesta
-- 
Siemens AG, T RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Henning Schild]

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>
---
 meta/classes/container-img.bbclass             |  4 +---
 meta/classes/image-container-extension.bbclass | 11 ++++++-----
 meta/classes/image-sdk-extension.bbclass       |  2 +-
 3 files changed, 8 insertions(+), 9 deletions(-)

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..43b67f9d864d 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_IMAGE_NAME ?= "${PN}-${DISTRO}-${DISTRO_ARCH}"
+CONTAINER_TAG ?= "${PV}-${PR}"
 
 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_IMAGE_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
 }
 
-- 
2.31.1

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

[Silvano Cirujano Cuesta]

On 28/07/2021 17:22, 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

This is a completely meaningful change. My implementation didn't foresee the need for free selection of container name and tag.

The default container name and tag that is patch is providing is also meaningful, but it's breaking the interface (resulting container image has a fixed, predefined name).
I don't have the impression that there are many users of this feature. Therefore we might afford it.
But I don't follow ISAR development close enough to be able to assess what's the impact of this change on the user community.

The alternative is breaking this transition in two steps:
1. Make the container image name and tag configurable, but keep the default backwards compatible (even if yours is more meaningful). And announce the upcoming breaking change.
2. Change the default effectively breaking the interface.

> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  meta/classes/container-img.bbclass             |  4 +---
>  meta/classes/image-container-extension.bbclass | 11 ++++++-----
>  meta/classes/image-sdk-extension.bbclass       |  2 +-
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> 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..43b67f9d864d 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_IMAGE_NAME ?= "${PN}-${DISTRO}-${DISTRO_ARCH}"
> +CONTAINER_TAG ?= "${PV}-${PR}"
>  
>  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_IMAGE_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

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

[Henning Schild]

Am Thu, 29 Jul 2021 14:50:12 +0200
schrieb Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>:

> On 28/07/2021 17:22, 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  
> 
> This is a completely meaningful change. My implementation didn't
> foresee the need for free selection of container name and tag.
> 
> The default container name and tag that is patch is providing is also
> meaningful, but it's breaking the interface (resulting container
> image has a fixed, predefined name). I don't have the impression that
> there are many users of this feature. Therefore we might afford it.
> But I don't follow ISAR development close enough to be able to assess
> what's the impact of this change on the user community.
> 
> The alternative is breaking this transition in two steps:
> 1. Make the container image name and tag configurable, but keep the
> default backwards compatible (even if yours is more meaningful). And
> announce the upcoming breaking change. 2. Change the default
> effectively breaking the interface.
 
I was expecting you to know users that might need to be warned about
the default name:tag change. But seems you are OK with it.

Let us see if anyone else speaks up, in which case we could see about
adding a way to stick to the old default.

Any way i will add an entry to the API-CHANGELOG to point out the
break. Maybe in v2 or just on top in case this v1 goes through as is.

Henning

> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  meta/classes/container-img.bbclass             |  4 +---
> >  meta/classes/image-container-extension.bbclass | 11 ++++++-----
> >  meta/classes/image-sdk-extension.bbclass       |  2 +-
> >  3 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > 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..43b67f9d864d 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_IMAGE_NAME ?= "${PN}-${DISTRO}-${DISTRO_ARCH}"
> > +CONTAINER_TAG ?= "${PV}-${PR}"
> >  
> >  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_IMAGE_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
> >  }
> >  
> >   

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

[Silvano Cirujano Cuesta]

On 29/07/2021 15:05, Henning Schild wrote:
> Am Thu, 29 Jul 2021 14:50:12 +0200
> schrieb Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>:
> 
>> On 28/07/2021 17:22, 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  
>>
>> This is a completely meaningful change. My implementation didn't
>> foresee the need for free selection of container name and tag.
>>
>> The default container name and tag that is patch is providing is also
>> meaningful, but it's breaking the interface (resulting container
>> image has a fixed, predefined name). I don't have the impression that
>> there are many users of this feature. Therefore we might afford it.
>> But I don't follow ISAR development close enough to be able to assess
>> what's the impact of this change on the user community.
>>
>> The alternative is breaking this transition in two steps:
>> 1. Make the container image name and tag configurable, but keep the
>> default backwards compatible (even if yours is more meaningful). And
>> announce the upcoming breaking change. 2. Change the default
>> effectively breaking the interface.
>  
> I was expecting you to know users that might need to be warned about
> the default name:tag change. But seems you are OK with it.

I don't know any users, I'm OK with it.

> 
> Let us see if anyone else speaks up, in which case we could see about
> adding a way to stick to the old default.

I agree with you that if there are no known users of this feature, we can afford this breaking change.

> 
> Any way i will add an entry to the API-CHANGELOG to point out the
> break. Maybe in v2 or just on top in case this v1 goes through as is.

Keeping the breaking change and adding the API-CHANGELOG entry would be in any case the lowest effort possible.

Silvano

> 
> Henning
> 
>>>
>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>> ---
>>>  meta/classes/container-img.bbclass             |  4 +---
>>>  meta/classes/image-container-extension.bbclass | 11 ++++++-----
>>>  meta/classes/image-sdk-extension.bbclass       |  2 +-
>>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>>
>>> 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..43b67f9d864d 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_IMAGE_NAME ?= "${PN}-${DISTRO}-${DISTRO_ARCH}"
>>> +CONTAINER_TAG ?= "${PV}-${PR}"
>>>  
>>>  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_IMAGE_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

[PATCH 4/4] classes: fix comment in container-img class

[Henning Schild]

The task name is in fact a different one, the function sits in another
class.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 meta/classes/container-img.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/classes/container-img.bbclass b/meta/classes/container-img.bbclass
index 9928a58ef53d..edd0e7f8f932 100644
--- a/meta/classes/container-img.bbclass
+++ b/meta/classes/container-img.bbclass
@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: MIT
 #
-# This class provides the task 'containerize_rootfs'
+# This class provides the task 'container_image'
 # to create container images containing the target rootfs.
 
 do_container_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
-- 
2.31.1

Re: [PATCH 4/4] classes: fix comment in container-img class

[Silvano Cirujano Cuesta]

On 28/07/2021 17:22, Henning Schild wrote:
> The task name is in fact a different one, the function sits in another
> class.

The error has historical reasons. But it's obvious (see the "do_container_image") that you're fully right.

> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  meta/classes/container-img.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/container-img.bbclass b/meta/classes/container-img.bbclass
> index 9928a58ef53d..edd0e7f8f932 100644
> --- a/meta/classes/container-img.bbclass
> +++ b/meta/classes/container-img.bbclass
> @@ -3,7 +3,7 @@
>  #
>  # SPDX-License-Identifier: MIT
>  #
> -# This class provides the task 'containerize_rootfs'
> +# This class provides the task 'container_image'
>  # to create container images containing the target rootfs.
>  
>  do_container_image[stamp-extra-info] = "${DISTRO}-${MACHINE}"
> 
-- 
  Silvano Cirujano Cuesta
-- 
Siemens AG, T RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 0/4] Allow better control over container tags

[Henning Schild]

This can also be found here:
 https://github.com/henning-schild-work/isar/tree/henning/staging

And so that CI picks it up here:
 https://github.com/henning-schild-work/isar/tree/henning/ilbers-ci

Henning

Am Wed, 28 Jul 2021 17:22:06 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> A container tags consists of "name:tag" and both have been kind of
> hardcoded so far. "tag" was always "latest" which is not proper
> versioning. And "name" has always been "isar-DISTRO-ARCH" which is
> also not proper because the name should probably describe the content
> and not so much the build system. Plus that name means guaranteed
> collision if you build two containers with isar based on the same
> disto and arch.
> 
> p3 is kind of interface breaking, i may have forgotten some docs and
> stuff
> 
> Henning Schild (4):
>   classes: make sure container extension can run multiple times
>   classes: simplify tag handling in container class
>   classes: allow more control over container image name and tag
>   classes: fix comment in container-img class
> 
>  meta/classes/container-img.bbclass            |  6 ++---
>  .../classes/image-container-extension.bbclass | 25
> ++++++++++--------- meta/classes/image-sdk-extension.bbclass      |
> 2 +- 3 files changed, 16 insertions(+), 17 deletions(-)
> 

Re: [PATCH 0/4] Allow better control over container tags

[Silvano Cirujano Cuesta]

Apart from the comment to Patch 3 regarding the breaking change that the default would produce that need some clarification, it looks good to me.

In any case, as you mention yourself, the breaking change (either direct on the application of the patch or later) needs to be well documented and announced. Also the variables CONTAINER_NAME and CONTAINER_TAG need to be documented.

Cheers,
  Silvano

On 28/07/2021 17:22, Henning Schild wrote:
> A container tags consists of "name:tag" and both have been kind of
> hardcoded so far. "tag" was always "latest" which is not proper
> versioning. And "name" has always been "isar-DISTRO-ARCH" which is also
> not proper because the name should probably describe the content and not
> so much the build system. Plus that name means guaranteed collision if
> you build two containers with isar based on the same disto and arch.
> 
> p3 is kind of interface breaking, i may have forgotten some docs and
> stuff
> 
> Henning Schild (4):
>   classes: make sure container extension can run multiple times
>   classes: simplify tag handling in container class
>   classes: allow more control over container image name and tag
>   classes: fix comment in container-img class
> 
>  meta/classes/container-img.bbclass            |  6 ++---
>  .../classes/image-container-extension.bbclass | 25 ++++++++++---------
>  meta/classes/image-sdk-extension.bbclass      |  2 +-
>  3 files changed, 16 insertions(+), 17 deletions(-)
> 
-- 
  Silvano Cirujano Cuesta
-- 
Siemens AG, T RDA IOT SES-DE
Corporate Competence Center Embedded Linux