[meta-swupdate,dunfell+] swupdate-common: get do_swuimage vardeps

Read all variables from sw-description file and add them to the vardeps
of the do_swuimage task. Bitbake cannot know that the do_swuimage task
which evaluates the templated sw-description file needs to be executed
if a variable which is refered by the sw-description file but not by
the recipe itself.
---
 classes/swupdate-common.bbclass | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Hi Adrian,

On 2021-09-07 10:45, Adrian Freihofer wrote:
> Read all variables from sw-description file and add them to the vardeps
> of the do_swuimage task. Bitbake cannot know that the do_swuimage task
> which evaluates the templated sw-description file needs to be executed
> if a variable which is refered by the sw-description file but not by
> the recipe itself.
> ---
>  classes/swupdate-common.bbclass | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/classes/swupdate-common.bbclass 
> b/classes/swupdate-common.bbclass
> index dabd466..cb3f084 100644
> --- a/classes/swupdate-common.bbclass
> +++ b/classes/swupdate-common.bbclass
> @@ -372,3 +372,26 @@ python do_swuimage () {
> 
>      swupdate_create_cpio(d, imgdeploydir, list_for_cpio)
>  }
> +
> +# Read all variables from sw-description file and add them to the
> vardeps of the do_swuimage task. Bitbake
> +# cannot know that the do_swuimage task which evaluates the templated
> sw-description file needs to be executed
> +# if a variable which is refered by the sw-description file but not
> by the recipe itself.
> +def swupdate_find_bitbake_variables(d):
> +    import re
> +
> +    vardeps = ''
> +    filespath = d.getVar('FILESPATH')
> +    sw_desc_path = bb.utils.which(filespath, "sw-description")

I find this a bit surprising hidden magic. I feel it would be better if 
the `sw-description` file was explicitly mentioned somewhere, before 
this does anything.

Also this prevents being able to fetch the `sw-description` from 
somewhere remote. But that is not preventable when doing it like this. 
But of course it is possible to fill the `vardeps` in manually in that 
case.

> +    try:
> +        with open(sw_desc_path, "r") as f:
> +            content_in = f.read()
> +            var_re = r"@@([^@].+)@@"
> +            m = re.findall(var_re, content_in)
> +            if m:
> +                vardeps = ' '.join(m)
> +    except IOError:
> +        # Since this runs at parse time where we don't know if this
> will be used later on we cannot threat this as an error
> +        bb.debug(1, "Cannot find sw-description. Please extend
> FILESPATH (%s)" % filespath)

Debug is too weak, at least `info` or even `warn` here, because the user 
does something unexpected for the recipe/class and need to know about it 
and fix it.

If someone wants to do something different, then they have to overwrite 
the `vardeps` flag anyway, and so preventing this function from being 
called. So I am in favor of the `warn` log level here.

regards,
Claudius

> +    return vardeps
> +
> +do_swuimage[vardeps] ?= "${@swupdate_find_bitbake_variables(d)}"
> --
> 2.31.1

Hello Claudius 

One of the most important goals of bitbake and Yocto is reproducibility. 
This is simply not guaranteed with the current implementation but would be 
with my patch. In the past I was struggeling over this issue several times: 
I changed a variable in bitbake but ended up with a sw-description file 
where still the old value was in. This is really wrong and needs to be 
fixed.

Fetching a file which has an impact on the task dependencies is not 
possible in general. Bitbake is parsing all the recipes and creating the 
task queues. After that the tasks are executed. It's not possible to have a 
task (e.g. a task running after do_fetch) which is going to modify the task 
queues later on.  This is also why bb.debug is correct. This lines of code 
run for each recipe which includes the swupdate-common.bbclass during parse 
time. Usually we do not want to get that many messages.

I see 3 potential solutions:
- The sw-description file cannot support variables but can be fetched.
- All the variables referred by sw-description file need to be added to the 
do_swuimage[vardeps] manually.
- My patch which provides a generic solution based on the assumption the 
sw-description is of type "file://"

regards,
Adrian
c...@denx.de schrieb am Dienstag, 7. September 2021 um 11:17:57 UTC+2:

> Hi Adrian,
>
> On 2021-09-07 10:45, Adrian Freihofer wrote:
> > Read all variables from sw-description file and add them to the vardeps
> > of the do_swuimage task. Bitbake cannot know that the do_swuimage task
> > which evaluates the templated sw-description file needs to be executed
> > if a variable which is refered by the sw-description file but not by
> > the recipe itself.
> > ---
> > classes/swupdate-common.bbclass | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> > 
> > diff --git a/classes/swupdate-common.bbclass 
> > b/classes/swupdate-common.bbclass
> > index dabd466..cb3f084 100644
> > --- a/classes/swupdate-common.bbclass
> > +++ b/classes/swupdate-common.bbclass
> > @@ -372,3 +372,26 @@ python do_swuimage () {
> > 
> > swupdate_create_cpio(d, imgdeploydir, list_for_cpio)
> > }
> > +
> > +# Read all variables from sw-description file and add them to the
> > vardeps of the do_swuimage task. Bitbake
> > +# cannot know that the do_swuimage task which evaluates the templated
> > sw-description file needs to be executed
> > +# if a variable which is refered by the sw-description file but not
> > by the recipe itself.
> > +def swupdate_find_bitbake_variables(d):
> > + import re
> > +
> > + vardeps = ''
> > + filespath = d.getVar('FILESPATH')
> > + sw_desc_path = bb.utils.which(filespath, "sw-description")
>
> I find this a bit surprising hidden magic. I feel it would be better if 
> the `sw-description` file was explicitly mentioned somewhere, before 
> this does anything.
>
> Also this prevents being able to fetch the `sw-description` from 
> somewhere remote. But that is not preventable when doing it like this. 
> But of course it is possible to fill the `vardeps` in manually in that 
> case.
>
> > + try:
> > + with open(sw_desc_path, "r") as f:
> > + content_in = f.read()
> > + var_re = r"@@([^@].+)@@"
> > + m = re.findall(var_re, content_in)
> > + if m:
> > + vardeps = ' '.join(m)
> > + except IOError:
> > + # Since this runs at parse time where we don't know if this
> > will be used later on we cannot threat this as an error
> > + bb.debug(1, "Cannot find sw-description. Please extend
> > FILESPATH (%s)" % filespath)
>
> Debug is too weak, at least `info` or even `warn` here, because the user 
> does something unexpected for the recipe/class and need to know about it 
> and fix it.
>
> If someone wants to do something different, then they have to overwrite 
> the `vardeps` flag anyway, and so preventing this function from being 
> called. So I am in favor of the `warn` log level here.
>
> regards,
> Claudius
>
> > + return vardeps
> > +
> > +do_swuimage[vardeps] ?= "${@swupdate_find_bitbake_variables(d)}"
> > --
> > 2.31.1
>

Hi Adrian, Claudius,

On 07.09.21 12:04, Adrian Freihofer wrote:
> Hello Claudius
> 
> One of the most important goals of bitbake and Yocto is reproducibility. 
> This is simply not guaranteed with the current implementation but would 
> be with my patch.

Currently, it should work, but do_swuimage always runs. However, this 
leads to the identical SWU.

> In the past I was struggeling over this issue several 
> times: I changed a variable in bitbake but ended up with a 
> sw-description file where still the old value was in. This is really 
> wrong and needs to be fixed.

Right.

> 
> Fetching a file which has an impact on the task dependencies is not 
> possible in general. Bitbake is parsing all the recipes and creating the 
> task queues. After that the tasks are executed. It's not possible to 
> have a task (e.g. a task running after do_fetch) which is going to 
> modify the task queues later on.

Just my experience by reworking the class: we cannot add any dependency 
to do_fetch() because this cause do_rootfs() to run again, and this is 
much worse as running do_swuimage. It is not only a question of time, 
but the resulting rootfs could be different due to some timestamping 
(not yet solved in OE) from the original one. And with large rootfs, it 
really takes a lot of time.

>  This is also why bb.debug is correct. 
> This lines of code run for each recipe which includes the 
> swupdate-common.bbclass during parse time. Usually we do not want to get 
> that many messages.

This is not an error if we use the swupdate class, too. A warn or error 
is then excluded.

> 
> I see 3 potential solutions:
> - The sw-description file cannot support variables but can be fetched.

IMHO the solution when sw-description must be fetched is to use the 
generic swupdate class instead of swupdate-image. swupdate-image is like 
a specialization of a generic class that just works for image recipes 
and have some constraints. We can add in doc that sw-description must be 
provided locally together with swupdate-image.

> - All the variables referred by sw-description file need to be added to 
> the do_swuimage[vardeps] manually.

IMO this leads to more drawbacks as advantages - it is very easy to 
forget to change the recipe after changing sw-description, and viceversa.

> - My patch which provides a generic solution based on the assumption the 
> sw-description is of type "file://"

My vote is to add to documentation if not clear. Currently, there is :

"Users just need to import the `swupdate-image` class. This already sets 
some variables.
  A sw-description must still be added into a `files` directory, that is 
automatically searched by the class.
"

If it is not enough, we can enforce the statement, reinforcing that 
sw-description must be provided locally, and use the swupdate class if 
it must be fetched.

> 
> regards,
> Adrian
> c...@denx.de schrieb am Dienstag, 7. September 2021 um 11:17:57 UTC+2:
> 
>     Hi Adrian,
> 
>     On 2021-09-07 10:45, Adrian Freihofer wrote:
>      > Read all variables from sw-description file and add them to the
>     vardeps
>      > of the do_swuimage task. Bitbake cannot know that the do_swuimage
>     task
>      > which evaluates the templated sw-description file needs to be
>     executed
>      > if a variable which is refered by the sw-description file but not by
>      > the recipe itself.
>      > ---
>      > classes/swupdate-common.bbclass | 23 +++++++++++++++++++++++
>      > 1 file changed, 23 insertions(+)
>      >
>      > diff --git a/classes/swupdate-common.bbclass
>      > b/classes/swupdate-common.bbclass
>      > index dabd466..cb3f084 100644
>      > --- a/classes/swupdate-common.bbclass
>      > +++ b/classes/swupdate-common.bbclass
>      > @@ -372,3 +372,26 @@ python do_swuimage () {
>      >
>      > swupdate_create_cpio(d, imgdeploydir, list_for_cpio)
>      > }
>      > +
>      > +# Read all variables from sw-description file and add them to the
>      > vardeps of the do_swuimage task. Bitbake
>      > +# cannot know that the do_swuimage task which evaluates the
>     templated
>      > sw-description file needs to be executed
>      > +# if a variable which is refered by the sw-description file but not
>      > by the recipe itself.
>      > +def swupdate_find_bitbake_variables(d):
>      > + import re
>      > +
>      > + vardeps = ''
>      > + filespath = d.getVar('FILESPATH')
>      > + sw_desc_path = bb.utils.which(filespath, "sw-description")
> 
>     I find this a bit surprising hidden magic. I feel it would be better if
>     the `sw-description` file was explicitly mentioned somewhere, before
>     this does anything.
> 
>     Also this prevents being able to fetch the `sw-description` from
>     somewhere remote. But that is not preventable when doing it like this.
>     But of course it is possible to fill the `vardeps` in manually in that
>     case.
> 
>      > + try:
>      > + with open(sw_desc_path, "r") as f:
>      > + content_in = f.read()
>      > + var_re = r"@@([^@].+)@@"
>      > + m = re.findall(var_re, content_in)
>      > + if m:
>      > + vardeps = ' '.join(m)
>      > + except IOError:
>      > + # Since this runs at parse time where we don't know if this
>      > will be used later on we cannot threat this as an error
>      > + bb.debug(1, "Cannot find sw-description. Please extend
>      > FILESPATH (%s)" % filespath)
> 
>     Debug is too weak, at least `info` or even `warn` here, because the
>     user
>     does something unexpected for the recipe/class and need to know
>     about it
>     and fix it.
> 
>     If someone wants to do something different, then they have to overwrite
>     the `vardeps` flag anyway, and so preventing this function from being
>     called. So I am in favor of the `warn` log level here.
> 
>     regards,
>     Claudius
> 
>      > + return vardeps
>      > +
>      > +do_swuimage[vardeps] ?= "${@swupdate_find_bitbake_variables(d)}"
>      > --
>      > 2.31.1
> 

Best regards,
Stefano

Hi Adrian,

On 07.09.21 10:45, Adrian Freihofer wrote:
> Read all variables from sw-description file and add them to the vardeps
> of the do_swuimage task. Bitbake cannot know that the do_swuimage task
> which evaluates the templated sw-description file needs to be executed
> if a variable which is refered by the sw-description file but not by
> the recipe itself.
> ---
>   classes/swupdate-common.bbclass | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/classes/swupdate-common.bbclass b/classes/swupdate-common.bbclass
> index dabd466..cb3f084 100644
> --- a/classes/swupdate-common.bbclass
> +++ b/classes/swupdate-common.bbclass
> @@ -372,3 +372,26 @@ python do_swuimage () {
>   
>       swupdate_create_cpio(d, imgdeploydir, list_for_cpio)
>   }
> +
> +# Read all variables from sw-description file and add them to the vardeps of the do_swuimage task. Bitbake
> +# cannot know that the do_swuimage task which evaluates the templated sw-description file needs to be executed
> +# if a variable which is refered by the sw-description file but not by the recipe itself.
> +def swupdate_find_bitbake_variables(d):
> +    import re
> +
> +    vardeps = ''
> +    filespath = d.getVar('FILESPATH')
> +    sw_desc_path = bb.utils.which(filespath, "sw-description")
> +    try:
> +        with open(sw_desc_path, "r") as f:
> +            content_in = f.read()
> +            var_re = r"@@([^@].+)@@"

This rule does not work for many case, specially in case of sha256 
together with other vaeriables. I tested this with a sw-description 
where there is these entries:

sha256 = 
"@@@APPLICATION_IMAGE@@-@@MACHINE@@@@SWUPDATE_IMAGES_FSTYPES[@@APPLICATION_IMAGE@@]@@";

your rule gives :

 >>> import re
 >>> re.findall(r"@@([^@].+)@@", 'sha256 = 
"@@@APPLICATION_IMAGE@@-@@MACHINE@@@@SWUPDATE_IMAGES_FSTYPES[@@APPLICATION_IMAGE@@]@@";')
['APPLICATION_IMAGE@@-@@MACHINE@@@@SWUPDATE_IMAGES_FSTYPES[@@APPLICATION_IMAGE@@]']
 >>>

it does not work here :-(


> +            m = re.findall(var_re, content_in)
> +            if m:
> +                vardeps = ' '.join(m)
> +    except IOError:
> +        # Since this runs at parse time where we don't know if this will be used later on we cannot threat this as an error
> +        bb.debug(1, "Cannot find sw-description. Please extend FILESPATH (%s)" % filespath)
> +    return vardeps
> +
> +do_swuimage[vardeps] ?= "${@swupdate_find_bitbake_variables(d)}"

This line should be better moved into the swupdate-image class. In that 
case, sw-description must be provided locally, and if not, it is an 
error - I will then exchange bb.debug with bb.error. If swupdate-image 
is not used, the function is not called at all, or it is under control 
of developer who must explicitely set vardeps manually or via the 
function. But he knows then.

Best regards,
Stefano