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 15:09:56 +0100
schrieb "Moessbauer, Felix (T RDA IOT SES-DE)"
<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.

do_rootfs_postprocess is executed before do_rootfs, you are right ...
that is some hefty naming ... later or sooner causing potential
problems ;)
But the real guy seems to be "rootfs_install" which installs everything
onto a cached bootstrap (not having a BUILD_ID).
If any debian packet changes rootfs_install should be re-run, and so
will rootfs_postprocess. If no "real" change happened, your BUILD_ID
will stay the same which so far i think was intended.

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

Might be a bad example .. but a "while(build) {}" would never terminate,
and that at least is a serious enough change to make it into the API
changelog.

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

I know that case, where you build a second time for a tag ... That is a
good example. While i would personally never build a release in a
"rebuild" but always on a clean cache. BUT would expect a non-clean
sstate cache to not cause harm.

To that point i guess we need a solution and it should be included in
another round of the sstate series.
If the solution is along the lines of this patch i would say that an
API changelog entry will be needed, and maybe revisit the user manual
what it has to say about the CMD.

Henning


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