From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 7026321669488640000 X-Received: by 2002:a05:6402:1151:: with SMTP id g17mr58861335edw.219.1635963383171; Wed, 03 Nov 2021 11:16:23 -0700 (PDT) X-BeenThere: isar-users@googlegroups.com Received: by 2002:aa7:c0d9:: with SMTP id j25ls1339905edp.3.gmail; Wed, 03 Nov 2021 11:16:22 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwj0cQ8gOH8iQfHnU+LuStWrtlNj03tMrvgryf5feTF7W5PaN8nYvGjdA8lPLohbQ2RMLoz X-Received: by 2002:a50:e0c3:: with SMTP id j3mr62090607edl.97.1635963382126; Wed, 03 Nov 2021 11:16:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1635963382; cv=none; d=google.com; s=arc-20160816; b=PqK/d2nxvtAsbSVGEyXtJsbKPHJhoNRJZjKb98ooS8WXzX7s+FXAa+L1FzdXl7m/Dd mPraTzXjOsfUneU3Lq8+X+fNVd4k2f56jR4mJ2Quj/K+u8ZVFPLQWvnUMUzb4KzwSnqv uUBv4nZbmzpLBWuHWtXooqeb24/Fzk+9ZkHRjtyPIOVZIi1vbdkz4bazSrbjysujjrs2 P/D2HrnG2lAlkwhut+YZ77uHl2dYhKi/f10NeYb1encCrzytyTPY2ZpOvnB7uCfjstMK 9BQzWFqCLgqgd5iHrPqmoshmx9NA7wLbm9fxXvSBEiZPsS37AR1Uj4i4y4H4W5BZ7Tgw cZnQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date; bh=xW9cY0RF+HpNXwfgb2VfaZfe393q5z2co9LRcrulfcM=; b=lJIU8rOlg/yS8+X9x7w4lowwGNugeHk6fO228F17ddnJk62yMZS0VqofiR5ZNO9HZm IUyLYZdYaVmoadihVlFZJGLn1UY2SMir9e8gZ/l1YEW+BrGQdD7jy2RVrpQAQzu3+/fx 7RAphZh0Eu4Lbn5OzP+qgQUT2m+p8Mv+21RXgKERyFntme425ycUBaBUF2CsN5nLN2B3 7giW6I6a8YE4LwFq/nvgD1p43xiVVD6cHEQ3AeWAwSmOz9KsSARsKYSOcgBIBflgHBr4 mJtTMLpozcP3myJHXjtQqgJVUwEdlQwjxcHLTRlMZEfrESbRbhdA7FeDIyo+Uc5+lzQl /2/w== ARC-Authentication-Results: i=1; gmr-mx.google.com; spf=pass (google.com: domain of henning.schild@siemens.com designates 192.35.17.2 as permitted sender) smtp.mailfrom=henning.schild@siemens.com; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=siemens.com Return-Path: Received: from thoth.sbs.de (thoth.sbs.de. [192.35.17.2]) by gmr-mx.google.com with ESMTPS id d2si202467edk.1.2021.11.03.11.16.22 for (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Nov 2021 11:16:22 -0700 (PDT) Received-SPF: pass (google.com: domain of henning.schild@siemens.com designates 192.35.17.2 as permitted sender) client-ip=192.35.17.2; Authentication-Results: gmr-mx.google.com; spf=pass (google.com: domain of henning.schild@siemens.com designates 192.35.17.2 as permitted sender) smtp.mailfrom=henning.schild@siemens.com; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=siemens.com Received: from mail1.sbs.de (mail1.sbs.de [192.129.41.35]) by thoth.sbs.de (8.15.2/8.15.2) with ESMTPS id 1A3IGLRg008277 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 3 Nov 2021 19:16:21 +0100 Received: from md1za8fc.ad001.siemens.net ([139.25.69.154]) by mail1.sbs.de (8.15.2/8.15.2) with ESMTP id 1A3IGLlc007231; Wed, 3 Nov 2021 19:16:21 +0100 Date: Wed, 3 Nov 2021 19:16:20 +0100 From: Henning Schild To: "Moessbauer, Felix (T RDA IOT SES-DE)" Cc: "isar-users@googlegroups.com" , "Schmidt, Adriaan (T RDA IOT SES-DE)" Subject: Re: [PATCH v3 1/2] always invoke ISAR_RELEASE_CMD to invalidate downstream tasks on change Message-ID: <20211103191620.786b9a9c@md1za8fc.ad001.siemens.net> In-Reply-To: References: <20211103123624.3015125-1-felix.moessbauer@siemens.com> <20211103123624.3015125-2-felix.moessbauer@siemens.com> <20211103144220.3c8e537e@md1za8fc.ad001.siemens.net> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TUID: N7DaDIvVHS8+ Am Wed, 3 Nov 2021 15:09:56 +0100 schrieb "Moessbauer, Felix (T RDA IOT SES-DE)" : > > -----Original Message----- > > From: Henning Schild > > Sent: Wednesday, November 3, 2021 2:42 PM > > To: Moessbauer, Felix (T RDA IOT SES-DE) > > Cc: isar-users@googlegroups.com; > > Schmidt, Adriaan (T RDA IOT SES-DE) > > 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 : > > > > > 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 > > > --- > > > 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) >