* [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD
@ 2021-11-03 12:36 Felix Moessbauer
2021-11-03 12:36 ` [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Felix Moessbauer
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Felix Moessbauer @ 2021-11-03 12:36 UTC (permalink / raw)
To: isar-users; +Cc: henning.schild, adriaan.schmidt, Felix Moessbauer
Changes since v2:
- fix bug in update_etc_os_release
- Strip newlines from ISAR_RELEASE_CMD
Changes since v1:
- remove obsolete get_build_id function
Felix Moessbauer (2):
always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on
change
Ensure generation of /etc/os-release is idempotent
meta/classes/image-postproc-extension.bbclass | 6 ++--
meta/classes/image.bbclass | 32 ++++++++++---------
meta/classes/rootfs.bbclass | 2 +-
3 files changed, 21 insertions(+), 19 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change 2021-11-03 12:36 [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Felix Moessbauer @ 2021-11-03 12:36 ` Felix Moessbauer 2021-11-03 13:42 ` Henning Schild 2021-11-03 12:36 ` [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent Felix Moessbauer 2021-11-03 18:20 ` [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Henning Schild 2 siblings, 1 reply; 9+ messages in thread From: Felix Moessbauer @ 2021-11-03 12:36 UTC (permalink / raw) To: isar-users; +Cc: henning.schild, adriaan.schmidt, Felix Moessbauer 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. When caching artifacts across builds (e.g. using sstate), this also affects fresh builds. 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. 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) -- 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change 2021-11-03 12:36 ` [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Felix Moessbauer @ 2021-11-03 13:42 ` Henning Schild 2021-11-03 14:09 ` Moessbauer, Felix 0 siblings, 1 reply; 9+ messages in thread From: Henning Schild @ 2021-11-03 13:42 UTC (permalink / raw) To: Felix Moessbauer; +Cc: isar-users, adriaan.schmidt 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change 2021-11-03 13:42 ` Henning Schild @ 2021-11-03 14:09 ` Moessbauer, Felix 2021-11-03 18:16 ` Henning Schild 0 siblings, 1 reply; 9+ messages in thread From: Moessbauer, Felix @ 2021-11-03 14:09 UTC (permalink / raw) To: henning.schild; +Cc: isar-users, Schmidt, Adriaan > -----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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change 2021-11-03 14:09 ` Moessbauer, Felix @ 2021-11-03 18:16 ` Henning Schild 2021-11-04 11:50 ` Schmidt, Adriaan 0 siblings, 1 reply; 9+ messages in thread From: Henning Schild @ 2021-11-03 18:16 UTC (permalink / raw) To: Moessbauer, Felix (T RDA IOT SES-DE) Cc: isar-users, Schmidt, Adriaan (T RDA IOT SES-DE) 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) > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change 2021-11-03 18:16 ` Henning Schild @ 2021-11-04 11:50 ` Schmidt, Adriaan 0 siblings, 0 replies; 9+ messages in thread From: Schmidt, Adriaan @ 2021-11-04 11:50 UTC (permalink / raw) To: henning.schild, Moessbauer, Felix; +Cc: isar-users Henning Schild, Mittwoch, 3. November 2021 19:16: > Am Wed, 3 Nov 2021 15:09:56 +0100 > schrieb "Moessbauer, Felix (T RDA IOT SES-DE)" > <felix.moessbauer@siemens.com>: > > > 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. I have tested this together with the sstate series, and I can confirm that the dependency on IMAGE_BUILD_ID is registered as expected, so in case the output of ISAR_RELEASE_CMD changes, the image will be rebuilt. We could pull this into the sstate patches, but I think it's less confusing if we don't. And ideally merge this before the sstate series. Adriaan ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent 2021-11-03 12:36 [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Felix Moessbauer 2021-11-03 12:36 ` [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Felix Moessbauer @ 2021-11-03 12:36 ` Felix Moessbauer 2021-11-03 13:46 ` Henning Schild 2021-11-03 18:20 ` [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Henning Schild 2 siblings, 1 reply; 9+ messages in thread From: Felix Moessbauer @ 2021-11-03 12:36 UTC (permalink / raw) To: isar-users; +Cc: henning.schild, adriaan.schmidt, Felix Moessbauer Prior to this patch, the generation of the os-release file was not idempotent as there was a name-glitch in VARIAN_VERSION. By that, the script did not detect an existing VARIANT_VERSION and add one additional one per invocation. Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> --- meta/classes/image-postproc-extension.bbclass | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/meta/classes/image-postproc-extension.bbclass b/meta/classes/image-postproc-extension.bbclass index 6e6b257..87749c8 100644 --- a/meta/classes/image-postproc-extension.bbclass +++ b/meta/classes/image-postproc-extension.bbclass @@ -27,7 +27,7 @@ update_etc_os_release() { sudo tee -a '${IMAGE_ROOTFS}/etc/os-release' fi if [ -n "${OS_RELEASE_VARIANT_VERSION}" ]; then - sudo sed -i '/^ISAR_IMAGE_VERSION=.*/d' '${IMAGE_ROOTFS}/etc/os-release' + sudo sed -i '/^VARIANT_VERSION=.*/d' '${IMAGE_ROOTFS}/etc/os-release' echo "VARIANT_VERSION=\"${PV}\"" | \ sudo tee -a '${IMAGE_ROOTFS}/etc/os-release' fi -- 2.30.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent 2021-11-03 12:36 ` [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent Felix Moessbauer @ 2021-11-03 13:46 ` Henning Schild 0 siblings, 0 replies; 9+ messages in thread From: Henning Schild @ 2021-11-03 13:46 UTC (permalink / raw) To: Felix Moessbauer; +Cc: isar-users, adriaan.schmidt, Gylstorff Quirin Am Wed, 3 Nov 2021 13:36:24 +0100 schrieb Felix Moessbauer <felix.moessbauer@siemens.com>: > Prior to this patch, the generation of the os-release file > was not idempotent as there was a name-glitch in VARIAN_VERSION. > By that, the script did not detect an existing VARIANT_VERSION > and add one additional one per invocation. > Fixes: 78b24427a050 ("classes/image-postproc: Add image version") > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> > --- > meta/classes/image-postproc-extension.bbclass | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/meta/classes/image-postproc-extension.bbclass > b/meta/classes/image-postproc-extension.bbclass index > 6e6b257..87749c8 100644 --- > a/meta/classes/image-postproc-extension.bbclass +++ > b/meta/classes/image-postproc-extension.bbclass @@ -27,7 +27,7 @@ > update_etc_os_release() { sudo tee -a '${IMAGE_ROOTFS}/etc/os-release' > fi > if [ -n "${OS_RELEASE_VARIANT_VERSION}" ]; then > - sudo sed -i '/^ISAR_IMAGE_VERSION=.*/d' > '${IMAGE_ROOTFS}/etc/os-release' > + sudo sed -i '/^VARIANT_VERSION=.*/d' > '${IMAGE_ROOTFS}/etc/os-release' echo "VARIANT_VERSION=\"${PV}\"" | \ > sudo tee -a '${IMAGE_ROOTFS}/etc/os-release' > fi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD 2021-11-03 12:36 [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Felix Moessbauer 2021-11-03 12:36 ` [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Felix Moessbauer 2021-11-03 12:36 ` [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent Felix Moessbauer @ 2021-11-03 18:20 ` Henning Schild 2 siblings, 0 replies; 9+ messages in thread From: Henning Schild @ 2021-11-03 18:20 UTC (permalink / raw) To: Felix Moessbauer; +Cc: isar-users, adriaan.schmidt I would even go so far to include a patch that renames do_rootfs_postprocess to do_rootfs_install_postprocess Henning Am Wed, 3 Nov 2021 13:36:22 +0100 schrieb Felix Moessbauer <felix.moessbauer@siemens.com>: > Changes since v2: > > - fix bug in update_etc_os_release > - Strip newlines from ISAR_RELEASE_CMD > > Changes since v1: > > - remove obsolete get_build_id function > > Felix Moessbauer (2): > always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on > change > Ensure generation of /etc/os-release is idempotent > > meta/classes/image-postproc-extension.bbclass | 6 ++-- > meta/classes/image.bbclass | 32 > ++++++++++--------- meta/classes/rootfs.bbclass | > 2 +- 3 files changed, 21 insertions(+), 19 deletions(-) > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-04 11:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-03 12:36 [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Felix Moessbauer 2021-11-03 12:36 ` [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Felix Moessbauer 2021-11-03 13:42 ` Henning Schild 2021-11-03 14:09 ` Moessbauer, Felix 2021-11-03 18:16 ` Henning Schild 2021-11-04 11:50 ` Schmidt, Adriaan 2021-11-03 12:36 ` [PATCH v3 2/2] Ensure generation of /etc/os-release is idempotent Felix Moessbauer 2021-11-03 13:46 ` Henning Schild 2021-11-03 18:20 ` [PATCH v3 0/2] Improve handling of ISAR_RELEASE_CMD Henning Schild
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox