Re: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change

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

Am Wed, 3 Nov 2021 13:36:23 +0100
schrieb Felix Moessbauer <felix.moessbauer@siemens.com>:

> The command in ISAR_RELEASE_CMD is used to add version information
> of the image under /etc/os-release. As we cannot predict the
> output of the command, we cannot cache it.
> Prior to this patch, this situation was just accepted resulting
> in misleading build-ids on incremental builds.

Yes, in fact i would further accept that instead of rebuilding.

> When caching artifacts across builds (e.g. using sstate),
> this also affects fresh builds.

Does it? It is an image postprocess step, and images are not sstate
cached. Unless we do image-in-package tricks for containers and VMs,
because packages will be sstate-cached.

> The patch ensures that ISAR_RELEASE_CMD is always invoked
> and the output is stored in the variable IMAGE_BUILD_ID.
> This variable is added as a vardep in the rootfs_postprocess
> task, so changes invalidate all downstream tasks.
>
> By that, we ensure that images always contain correct release
> information.

It is really hard to say what is correct. If a git change does not
trigger the image to be rebuild ... the code change very likely does
not do anything anyways. So whichever "git describe" comes out, both
are equivavlent and kind of "correct". And you save the second
"pointless" build.

I have seen layers that "echo $git_describe $date", those would never
be "correct" ;). And say you do not commit you will only get
"sha-dirty" but never "sha-very-dirty".

Not sure this patch is valid. Would it rebuild if i did "touch hello;
git add hello; git commit -m hello" ... so not change anything that
would affect the image? Or would it even rebuild every single time?

Henning

> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>  meta/classes/image-postproc-extension.bbclass |  4 +--
>  meta/classes/image.bbclass                    | 32
> ++++++++++--------- meta/classes/rootfs.bbclass                   |
> 2 +- 3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/meta/classes/image-postproc-extension.bbclass
> b/meta/classes/image-postproc-extension.bbclass index
> 3f00c21..6e6b257 100644 ---
> a/meta/classes/image-postproc-extension.bbclass +++
> b/meta/classes/image-postproc-extension.bbclass @@ -46,11 +46,11 @@
> image_postprocess_configure() { }
>  
>  ROOTFS_POSTPROCESS_COMMAND =+ "image_postprocess_mark"
> +ROOTFS_POSTPROCESS_VARDEPS =+ "IMAGE_BUILD_ID"
>  
>  image_postprocess_mark() {
> -    BUILD_ID=$(get_build_id)
>      update_etc_os_release \
> -        --build-id "${BUILD_ID}" --variant "${DESCRIPTION}"
> --version "${PV}"
> +        --build-id "${IMAGE_BUILD_ID}" --variant "${DESCRIPTION}"
> --version "${PV}" }
>  
>  ROOTFS_POSTPROCESS_COMMAND =+ "image_postprocess_machine_id"
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 5a0f32e..36883a8 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -92,21 +92,23 @@ def get_rootfs_size(d):
>  
>      return base_size + rootfs_extra * 1024
>  
> -# here we call a command that should describe your whole build
> system, -# this could be "git describe" or something similar.
> -# set ISAR_RELEASE_CMD to customize, or override do_mark_rootfs to
> do something -# completely different
> -get_build_id() {
> -	if [ $(echo ${BBLAYERS} | wc -w) -ne 2 ] &&
> -	   [ "${ISAR_RELEASE_CMD}" = "${ISAR_RELEASE_CMD_DEFAULT}"
> ]; then
> -		bbwarn "You are using external layers that will not
> be" \
> -		       "considered in the build_id. Consider
> changing" \
> -		       "ISAR_RELEASE_CMD."
> -	fi
> -	if ! ( ${ISAR_RELEASE_CMD} ) 2>/dev/null; then
> -		bbwarn "\"${ISAR_RELEASE_CMD}\" failed, returning
> empty build_id."
> -		echo ""
> -	fi
> +# the IMAGE_BUILD_ID depends on external conditions injected via
> +# ISAR_RELEASE_CMD. By that, we have to compute the value
> +# on each invocation
> +python() {
> +    bblayers = d.getVar('BBLAYERS', True)
> +    release_cmd = d.getVar('ISAR_RELEASE_CMD', True)
> +    if len(bblayers.split()) != 2:
> +        if release_cmd == d.getVar('ISAR_RELEASE_CMD_DEFAULT', True):
> +            bb.warn('You are using external layers that will not be '
> +                    'considered in the build_id. Consider changing '
> +                    'ISAR_RELEASE_CMD.')
> +    try:
> +        out,err = bb.process.run(release_cmd)
> +        d.setVar('IMAGE_BUILD_ID', out.replace('\n', ''))
> +    except(bb.process.ExecutionError):
> +        bb.warn(f'"{release_cmd}" failed, returning empty build_id.')
> +        d.setVar('IMAGE_BUILD_ID', '')
>  }
>  
>  python set_image_size () {
> diff --git a/meta/classes/rootfs.bbclass b/meta/classes/rootfs.bbclass
> index 20ccb00..e8c7fa0 100644
> --- a/meta/classes/rootfs.bbclass
> +++ b/meta/classes/rootfs.bbclass
> @@ -239,7 +239,7 @@ rootfs_export_dpkg_status() {
>         '${ROOTFS_DPKGSTATUS_DEPLOY_DIR}'/'${PF}'.dpkg_status
>  }
>  
> -do_rootfs_postprocess[vardeps] = "${ROOTFS_POSTPROCESS_COMMAND}"
> +do_rootfs_postprocess[vardeps] = "${ROOTFS_POSTPROCESS_COMMAND}
> ${ROOTFS_POSTPROCESS_VARDEPS}" python do_rootfs_postprocess() {
>      # Take care that its correctly mounted:
>      bb.build.exec_func('rootfs_do_mounts', d)