Message ID | 20210907084554.2912694-1-adrian.freihofer@siemens.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [meta-swupdate,dunfell+] swupdate-common: get do_swuimage vardeps | expand |
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
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"@@([^@].+)@@" + 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)}"