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

["Moessbauer, Felix" <felix.moessbauer@siemens.com>]

> -----Original Message-----
> From: Henning Schild <henning.schild@siemens.com>
> Sent: Wednesday, November 3, 2021 2:42 PM
> To: Moessbauer, Felix (T RDA IOT SES-DE) <felix.moessbauer@siemens.com>
> Cc: isar-users@googlegroups.com; Schmidt, Adriaan (T RDA IOT SES-DE)
> <adriaan.schmidt@siemens.com>
> Subject: Re: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate
> downstream tasks on change
> 
> 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.

IMO this is just wrong.
If this is the intended behavior, the ISAR_RELEASE_CMD should be written in a way that only reflects code changes.

> 
> > 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.

It does, as the SState caches / replaces the do_rootfs task.
The task that modifies /etc/os-release is do_rootfs_postprocess that is executed BEFORE do_rootfs.
By that /etc/os-release ends up in the cached state.

> 
> > 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".

Well, they are indeed correct. The date corresponds to the build date and that points to a time during the last image build.
Also the content of the ISAR_RELEASE_CMD is not something ISAR should reason about.
The only guarantee ISAR should provide is that the command is used to generate the image version
which finally ends up in the image (value is computed at build time).

> 
> 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?

Right, but then not the patch is invalid, but your ISAR_RELEASE_CMD is not well written.
A common use-case where I ended up with incorrect tags is the following:

- build the Image
- test the image
- add a git tag (e.g. v1.0.0)
- rebuild & deploy

Here, the deployed image contains the wrong tag.
When building with SState, even deleting the build/tmp folder does not help as everything is already in the cache.

Felix

> 
> 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)