[OE-core][RFC PATCH 0/2] introduce multi-state image deployment

[OE-core][RFC PATCH 0/2] introduce multi-state image deployment

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hi Richard,

please take a look at the following patch series. It's a follow up to
my previous attempt[1] and is also related to the dm-verity problem I
described on the list[2].

This time instead of having hardcoded two stages of deployment for simple
and composed image types, each image task becomes an sstate task and
handles the deployment of the artifacts it generates. Files created by
tasks other than IMAGE_CMD are still deployed by do_image_complete.

I'm tagging this series as RFC but actually the code turned out to be
surprisingly clean and straightforward. Please let me know what you
think.

[1] https://lists.openembedded.org/g/openembedded-core/message/136526
[2] https://lists.openembedded.org/g/openembedded-core/message/136203

Bartosz Golaszewski (2):
  qemuboot.bbclass: don't redefine IMGDEPLOYDIR
  image.bbclass: deploy image artifacts in stages

 meta/classes/image.bbclass    | 26 +++++++++++++++++++++++++-
 meta/classes/qemuboot.bbclass |  1 -
 2 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.19.1

[OE-core][RFC PATCH 1/2] qemuboot.bbclass: don't redefine IMGDEPLOYDIR

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This variable is already defined in image.bbclass and there's not need
to redefine it here.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 meta/classes/qemuboot.bbclass | 1 -
 1 file changed, 1 deletion(-)

diff --git a/meta/classes/qemuboot.bbclass b/meta/classes/qemuboot.bbclass
index 54044c38da..68f7a03619 100644
--- a/meta/classes/qemuboot.bbclass
+++ b/meta/classes/qemuboot.bbclass
@@ -83,7 +83,6 @@ QB_DRIVE_TYPE ?= "/dev/sd"
 
 # Create qemuboot.conf
 addtask do_write_qemuboot_conf after do_rootfs before do_image
-IMGDEPLOYDIR ?= "${WORKDIR}/deploy-${PN}-image-complete"
 
 def qemuboot_vars(d):
     build_vars = ['MACHINE', 'TUNE_ARCH', 'DEPLOY_DIR_IMAGE',
-- 
2.19.1

Re: [OE-core][RFC PATCH 1/2] qemuboot.bbclass: don't redefine IMGDEPLOYDIR

[Bartosz Golaszewski]

pon., 23 mar 2020 o 18:28 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This variable is already defined in image.bbclass and there's not need
> to redefine it here.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  meta/classes/qemuboot.bbclass | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/meta/classes/qemuboot.bbclass b/meta/classes/qemuboot.bbclass
> index 54044c38da..68f7a03619 100644
> --- a/meta/classes/qemuboot.bbclass
> +++ b/meta/classes/qemuboot.bbclass
> @@ -83,7 +83,6 @@ QB_DRIVE_TYPE ?= "/dev/sd"
>
>  # Create qemuboot.conf
>  addtask do_write_qemuboot_conf after do_rootfs before do_image
> -IMGDEPLOYDIR ?= "${WORKDIR}/deploy-${PN}-image-complete"
>
>  def qemuboot_vars(d):
>      build_vars = ['MACHINE', 'TUNE_ARCH', 'DEPLOY_DIR_IMAGE',
> --
> 2.19.1
>

Hi Richard,

this patch can be applied without waiting for the v2 of the image
deployment patch. Would you mind picking it up into your tree?

Bartosz

[OE-core][RFC PATCH 2/2] image.bbclass: deploy image artifacts in stages

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Make each IMAGE_CMD task an sstate task with its own IMGDEPLOYDIR
override. This way each generated set of artifacts is deployed as soon
as it's ready instead of the do_image_complete task handling the entire
deployement. This allows us to better fine-tune dependencies e.g. we
can make do_image_wic depend on fitImage task which can in turn depend
on do_image_ext4.

We need to completely delete the do_package task in order to avoid
problems with task hash changes as well as delete the IMGDEPLOYDIR
variable from the data object passed to each image task so that it's
expanded with the correct override.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 meta/classes/image.bbclass | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 07aa1f1fa5..3487e2abe4 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -428,10 +428,12 @@ python () {
         # date/time values. It will get expanded at execution time.
         # Similarly TMPDIR since otherwise we see QA stamp comparision problems
         # Expand PV else it can trigger get_srcrev which can fail due to these variables being unset
+        # Delete IMGDEPLOYDIR so that each task gets its own, overriden value
         localdata.setVar('PV', d.getVar('PV'))
         localdata.delVar('DATETIME')
         localdata.delVar('DATE')
         localdata.delVar('TMPDIR')
+        localdata.delVar('IMGDEPLOYDIR')
         vardepsexclude = (d.getVarFlag('IMAGE_CMD_' + realt, 'vardepsexclude', True) or '').split()
         for dep in vardepsexclude:
             localdata.delVar(dep)
@@ -502,6 +504,28 @@ python () {
 
         bb.debug(2, "Adding task %s before %s, after %s" % (task, 'do_image_complete', after))
         bb.build.addtask(task, 'do_image_complete', after, d)
+
+        imgdeploydir = d.getVar('IMGDEPLOYDIR')
+        task_override = task[3:].replace('_', '-') # 'do_image_ext4' becomes 'image-ext4'
+        taskdeploydir = '{}/deploy-{}-{}'.format(os.path.dirname(imgdeploydir),
+                                                 d.getVar('PN'), task_override)
+
+        # Each image task gets its own IMGDEPLOYDIR and becomes part of SSTATETASKS
+        # so that each set of artifacts be deployed right after the task completes.
+        d.setVar('IMGDEPLOYDIR_task-{}'.format(task_override), taskdeploydir)
+        d.appendVar('SSTATETASKS', ' {}'.format(task))
+        d.setVarFlag(task, 'sstate-inputdirs', taskdeploydir)
+        d.setVarFlag(task, 'sstate-outputdirs', d.getVar('DEPLOY_DIR_IMAGE'))
+        d.setVarFlag(task, 'cleandirs', taskdeploydir)
+        d.setVar('SSTATE_SKIP_CREATION_task-{}'.format(task_override), '1')
+
+        # FIXME We need to do this manually if we want to set the sstate flags from
+        # python code. The reason for this is that sstate.bbclass does this from an
+        # annonymous python function too and we can't guarantee correct ordering.
+        # This isn't a problem for most recipes as they define sstate tasks statically
+        # with bitbake variables.
+        d.prependVarFlag(task, 'prefuncs', 'sstate_task_prefunc ')
+        d.appendVarFlag(task, 'postfuncs', ' sstate_task_postfunc')
 }
 
 #
@@ -611,7 +635,7 @@ do_compile[noexec] = "1"
 do_install[noexec] = "1"
 deltask do_populate_lic
 deltask do_populate_sysroot
-do_package[noexec] = "1"
+deltask do_package
 deltask do_package_qa
 do_packagedata[noexec] = "1"
 deltask do_package_write_ipk
-- 
2.19.1

Re: [OE-core][RFC PATCH 2/2] image.bbclass: deploy image artifacts in stages

[Bartosz Golaszewski]

pon., 23 mar 2020 o 18:28 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Make each IMAGE_CMD task an sstate task with its own IMGDEPLOYDIR
> override. This way each generated set of artifacts is deployed as soon
> as it's ready instead of the do_image_complete task handling the entire
> deployement. This allows us to better fine-tune dependencies e.g. we
> can make do_image_wic depend on fitImage task which can in turn depend
> on do_image_ext4.
>
> We need to completely delete the do_package task in order to avoid
> problems with task hash changes as well as delete the IMGDEPLOYDIR
> variable from the data object passed to each image task so that it's
> expanded with the correct override.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Hi all!

It's been a week. Can I get some feedback on this? Is the idea at
least remotely acceptable/can we build upon it?

Bart

Re: [OE-core][RFC PATCH 2/2] image.bbclass: deploy image artifacts in stages

[Richard Purdie]

On Mon, 2020-03-30 at 18:26 +0200, Bartosz Golaszewski wrote:
> pon., 23 mar 2020 o 18:28 Bartosz Golaszewski <brgl@bgdev.pl>
> napisał(a):
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > 
> > Make each IMAGE_CMD task an sstate task with its own IMGDEPLOYDIR
> > override. This way each generated set of artifacts is deployed as
> > soon
> > as it's ready instead of the do_image_complete task handling the
> > entire
> > deployement. This allows us to better fine-tune dependencies e.g.
> > we
> > can make do_image_wic depend on fitImage task which can in turn
> > depend
> > on do_image_ext4.
> > 
> > We need to completely delete the do_package task in order to avoid
> > problems with task hash changes as well as delete the IMGDEPLOYDIR
> > variable from the data object passed to each image task so that
> > it's
> > expanded with the correct override.
> > 
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> It's been a week. Can I get some feedback on this? Is the idea at
> least remotely acceptable/can we build upon it?

FWIW at a quick glance I didn't think it was too bad.

I think there will be corner cases to resolve which I was hoping to
look into and give some pointers to but I haven't had the time.

I'm hoping somehow we can improve the FIXME you mention too.

The do_package change should probably be separated out - I'm guessing
we did noexec there for a reason though?

Also, you used {}.format which I'm torn on since most of the codebase
uses the other approach.

Its too late to get this into 3.1 and that is where I'm focusing my
effort right now but I think this is probably going the right way.

Cheers,

Richard

Re: [OE-core][RFC PATCH 2/2] image.bbclass: deploy image artifacts in stages

[Bartosz Golaszewski]

pon., 30 mar 2020 o 18:31 Richard Purdie
<richard.purdie@linuxfoundation.org> napisał(a):
>
> On Mon, 2020-03-30 at 18:26 +0200, Bartosz Golaszewski wrote:
> > pon., 23 mar 2020 o 18:28 Bartosz Golaszewski <brgl@bgdev.pl>
> > napisał(a):
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Make each IMAGE_CMD task an sstate task with its own IMGDEPLOYDIR
> > > override. This way each generated set of artifacts is deployed as
> > > soon
> > > as it's ready instead of the do_image_complete task handling the
> > > entire
> > > deployement. This allows us to better fine-tune dependencies e.g.
> > > we
> > > can make do_image_wic depend on fitImage task which can in turn
> > > depend
> > > on do_image_ext4.
> > >
> > > We need to completely delete the do_package task in order to avoid
> > > problems with task hash changes as well as delete the IMGDEPLOYDIR
> > > variable from the data object passed to each image task so that
> > > it's
> > > expanded with the correct override.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > It's been a week. Can I get some feedback on this? Is the idea at
> > least remotely acceptable/can we build upon it?
>
> FWIW at a quick glance I didn't think it was too bad.
>
> I think there will be corner cases to resolve which I was hoping to
> look into and give some pointers to but I haven't had the time.
>
> I'm hoping somehow we can improve the FIXME you mention too.
>

Yeah, after thinking about it more, I figured that if a task signature
changes, then something is wrong. I guess it's because we end up
assigning pre- and postfuncs to tasks twice: once in anonymous python
code from image.bbclass and then from sstate.bbclass. Maybe we should
provide a python helper for explicitly making a task an sstate task
and - when using it - not add said task to SSTATETASKS, so that
sstate.bbclass code ignores it? Or maybe this anonymous function
should become an event handler invoked at event.RecipeParsed?

> The do_package change should probably be separated out - I'm guessing
> we did noexec there for a reason though?
>

I couldn't find a reason. Maybe it's a leftover from some previous,
now cleaned up change.

> Also, you used {}.format which I'm torn on since most of the codebase
> uses the other approach.
>

I couldn't find any official guidelines for that. {} is the preferred
way in Python. Maybe it's time to start slowly converting the bitbake
codebase?

> Its too late to get this into 3.1 and that is where I'm focusing my
> effort right now but I think this is probably going the right way.
>

Sure, I didn't expect it to go into the next release. If you're happy
with this direction I'll try to improve it and send a v2.

Bartosz

Re: [OE-core][RFC PATCH 2/2] image.bbclass: deploy image artifacts in stages

[Peter Kjellerstedt]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-
> core@lists.openembedded.org> On Behalf Of Bartosz Golaszewski
> Sent: den 31 mars 2020 11:43
> To: Richard Purdie <richard.purdie@linuxfoundation.org>
> Cc: Khem Raj <raj.khem@gmail.com>; Armin Kuster <akuster808@gmail.com>;
> Jerome Neanne <jneanne@baylibre.com>; Quentin Schulz
> <quentin.schulz@streamunlimited.com>; Patches and discussions about the
> oe-core layer <openembedded-core@lists.openembedded.org>; Bartosz
> Golaszewski <bgolaszewski@baylibre.com>
> Subject: Re: [OE-core][RFC PATCH 2/2] image.bbclass: deploy image
> artifacts in stages
> 
> pon., 30 mar 2020 o 18:31 Richard Purdie
> <richard.purdie@linuxfoundation.org> napisał(a):

[cut]

> > Also, you used {}.format which I'm torn on since most of the codebase
> > uses the other approach.
> 
> I couldn't find any official guidelines for that. {} is the preferred
> way in Python. Maybe it's time to start slowly converting the bitbake
> codebase?

If we have waited so long to change string formatting, why not wait 
till we require  Python 3.6 and go for f-strings directly? With Tim's 
proposed patch we will soon require Python 3.5 as a minimum, so 3.6 
as a minimum requirement is probably not too long in the future...

Anyway, since I had fun generating it, I have included a graph 
depicting the use of % vs .format() for all Poky releases (and the 
script used to generate it; run it in a pristine clone of Poky for 
an interactive experience). Enjoy. :)

(For the curious, the first use of .format() was introduced to 
bitbake almost exactly 10 years ago in the Laverne release.)

//Peter


[-- Attachment #2: formattingwar.png --]
[-- Type: image/png, Size: 33616 bytes --]

[-- Attachment #3: formattingwar-plot.sh --]
[-- Type: application/octet-stream, Size: 1365 bytes --]

#!/bin/sh

REVS=" \
inky-1.0 \
clyde-2.0 \
blinky-3.0 \
pinky-3.1 \
purple-3.2 \
green-3.3 \
laverne-4.0 \
bernard-5.0 \
edison-6.0 \
denzil-7.0 \
danny-8.0 \
dylan-9.0.0 \
dora-10.0.0 \
daisy-11.0.0 \
dizzy-12.0.0 \
fido-13.0.0 \
jethro-14.0.0 \
krogoth-15.0.0 \
morty-16.0.0 \
pyro-17.0.0 \
rocko-18.0.0 \
sumo-19.0.0 \
thud-20.0.0 \
warrior-21.0.0 \
zeus-22.0.0 \
master \
"

gather_data() {
	local name=$1 rev=$2

	git checkout -f $rev >/dev/null 2>&1
	printf "%s\t%s\t%d\t%d\n" \
		$name \
		$(git show -s --format=%cd --date=format:%Y-%m-%d) \
		$(grep --exclude-dir=.git --exclude='*.patch' -ri '["'\''] % ' | wc -l) \
		$(grep --exclude-dir=.git --exclude='*.patch' -ri '\.format(' | wc -l)
}

[ -f formattingwar.dat ] || {
	for rev in $REVS; do
		release=${rev%-*}
		gather_data $release $rev
	done
} > formattingwar.dat

git checkout master >/dev/null 2>&1

gnuplot <<-EOF
	# Uncomment the following two lines to write the graph to a file.
#	set terminal pngcairo size 800,600
#	set output "formattingwar.png"

	set title "Use of % vs .format()"
	set xtics rotate by 45 right
	set key noenhanced
	set style data linespoints
	set timefmt "%Y-%m-%d"
	set xdata time
	set mouse mouseformat 3
	plot 'formattingwar.dat' using 2:3:xtic(1) title '%', \
	     ''                  using 2:4         title '.format()'

	pause mouse close
EOF

#rm formattingwar.dat