Patchwork image.bbclass: Ensure IMAGE_FSTYPES is fully evaluated before live/live logic

login
register
mail settings
Submitter Tom Rini
Date June 26, 2014, 12:27 p.m.
Message ID <1403785652-17065-1-git-send-email-tom.rini@gmail.com>
Download mbox | patch
Permalink /patch/74409/
State New
Headers show

Comments

Tom Rini - June 26, 2014, 12:27 p.m.
Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
that we evaluate it fully before performing the live and vmdk logic
checks.

Signed-off-by: Tom Rini <tom.rini@gmail.com>
---
 meta/classes/image.bbclass | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Otavio Salvador - June 26, 2014, 5:25 p.m.
On Thu, Jun 26, 2014 at 9:27 AM, Tom Rini <tom.rini@gmail.com> wrote:
> Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
> that we evaluate it fully before performing the live and vmdk logic
> checks.
>
> Signed-off-by: Tom Rini <tom.rini@gmail.com>

Do you have an example how it used to fail?
Richard Purdie - June 27, 2014, 1:40 p.m.
On Thu, 2014-06-26 at 08:27 -0400, Tom Rini wrote:
> Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
> that we evaluate it fully before performing the live and vmdk logic
> checks.
> 
> Signed-off-by: Tom Rini <tom.rini@gmail.com>
> ---
>  meta/classes/image.bbclass | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 79de5a2..ccfa1b1 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -78,10 +78,11 @@ do_rootfs[vardeps] += "BAD_RECOMMENDATIONS NO_RECOMMENDATIONS"
>  
>  do_build[depends] += "virtual/kernel:do_deploy"
>  
> +EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
>  def build_live(d):
> -    if base_contains("IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> -        d.setVar('NOISO', base_contains('IMAGE_FSTYPES', "iso", "0", "1", d))
> -        d.setVar('NOHDD', base_contains('IMAGE_FSTYPES', "hddimg", "0", "1", d))
> +    if base_contains("EVALED_IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> +        d.setVar('NOISO', base_contains('EVALED_IMAGE_FSTYPES', "iso", "0", "1", d))
> +        d.setVar('NOHDD', base_contains('EVALED_IMAGE_FSTYPES', "hddimg", "0", "1", d))
>          if d.getVar('NOISO', True) == "0" or d.getVar('NOHDD', True) == "0":
>              return "image-live"
>          return ""

The trouble is this doesn't add up. 

EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"

basically sets EVALED_... to the expanded value of IMAGE_FSTYPES

base_contains() also expands the variable its passed.

I'm a little concerned as to why those two expansions of the same thing
should result in something different.

From your later explaination, it would appear the use of the override is
causing problems but I don't understand how immediate expansion would
work around that :/.

So before this merges, more investigation is needed as there is
something not adding up...

Cheers,

Richard


> @@ -90,7 +91,7 @@ def build_live(d):
>  IMAGE_TYPE_live = "${@build_live(d)}"
>  
>  inherit ${IMAGE_TYPE_live}
> -IMAGE_TYPE_vmdk = '${@base_contains("IMAGE_FSTYPES", "vmdk", "image-vmdk", "", d)}'
> +IMAGE_TYPE_vmdk = '${@base_contains("EVALED_IMAGE_FSTYPES", "vmdk", "image-vmdk", "", d)}'
>  inherit ${IMAGE_TYPE_vmdk}
>  
>  python () {
> -- 
> 1.9.3
>
Tom Rini - June 27, 2014, 2:16 p.m.
On Fri, Jun 27, 2014 at 02:40:44PM +0100, Richard Purdie wrote:
> On Thu, 2014-06-26 at 08:27 -0400, Tom Rini wrote:
> > Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
> > that we evaluate it fully before performing the live and vmdk logic
> > checks.
> > 
> > Signed-off-by: Tom Rini <tom.rini@gmail.com>
> > ---
> >  meta/classes/image.bbclass | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> > index 79de5a2..ccfa1b1 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -78,10 +78,11 @@ do_rootfs[vardeps] += "BAD_RECOMMENDATIONS NO_RECOMMENDATIONS"
> >  
> >  do_build[depends] += "virtual/kernel:do_deploy"
> >  
> > +EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> >  def build_live(d):
> > -    if base_contains("IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > -        d.setVar('NOISO', base_contains('IMAGE_FSTYPES', "iso", "0", "1", d))
> > -        d.setVar('NOHDD', base_contains('IMAGE_FSTYPES', "hddimg", "0", "1", d))
> > +    if base_contains("EVALED_IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > +        d.setVar('NOISO', base_contains('EVALED_IMAGE_FSTYPES', "iso", "0", "1", d))
> > +        d.setVar('NOHDD', base_contains('EVALED_IMAGE_FSTYPES', "hddimg", "0", "1", d))
> >          if d.getVar('NOISO', True) == "0" or d.getVar('NOHDD', True) == "0":
> >              return "image-live"
> >          return ""
> 
> The trouble is this doesn't add up. 
> 
> EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> 
> basically sets EVALED_... to the expanded value of IMAGE_FSTYPES
> 
> base_contains() also expands the variable its passed.
> 
> I'm a little concerned as to why those two expansions of the same thing
> should result in something different.

Well, they're happening at different parts in the parse.  The vmdk
example is clearer I think since it's not another "big" python function.

> From your later explaination, it would appear the use of the override is
> causing problems but I don't understand how immediate expansion would
> work around that :/.
> 
> So before this merges, more investigation is needed as there is
> something not adding up...

Well, I think the high level answer is that no, we aren't applying
OVERRIDES at the point when we're parsing classes still rather than a
specific task.  Adding an immediate eval at this level does force things
and then we see what we're supposed to.  My gut feeling here is that if
we poke deeply enough at base_contains we'll see that we aren't passing
along the expand flag when we thought we were.
Richard Purdie - June 27, 2014, 2:54 p.m.
On Fri, 2014-06-27 at 10:16 -0400, Tom Rini wrote:
> On Fri, Jun 27, 2014 at 02:40:44PM +0100, Richard Purdie wrote:
> > On Thu, 2014-06-26 at 08:27 -0400, Tom Rini wrote:
> > > Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
> > > that we evaluate it fully before performing the live and vmdk logic
> > > checks.
> > > 
> > > Signed-off-by: Tom Rini <tom.rini@gmail.com>
> > > ---
> > >  meta/classes/image.bbclass | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> > > index 79de5a2..ccfa1b1 100644
> > > --- a/meta/classes/image.bbclass
> > > +++ b/meta/classes/image.bbclass
> > > @@ -78,10 +78,11 @@ do_rootfs[vardeps] += "BAD_RECOMMENDATIONS NO_RECOMMENDATIONS"
> > >  
> > >  do_build[depends] += "virtual/kernel:do_deploy"
> > >  
> > > +EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> > >  def build_live(d):
> > > -    if base_contains("IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > > -        d.setVar('NOISO', base_contains('IMAGE_FSTYPES', "iso", "0", "1", d))
> > > -        d.setVar('NOHDD', base_contains('IMAGE_FSTYPES', "hddimg", "0", "1", d))
> > > +    if base_contains("EVALED_IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > > +        d.setVar('NOISO', base_contains('EVALED_IMAGE_FSTYPES', "iso", "0", "1", d))
> > > +        d.setVar('NOHDD', base_contains('EVALED_IMAGE_FSTYPES', "hddimg", "0", "1", d))
> > >          if d.getVar('NOISO', True) == "0" or d.getVar('NOHDD', True) == "0":
> > >              return "image-live"
> > >          return ""
> > 
> > The trouble is this doesn't add up. 
> > 
> > EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> > 
> > basically sets EVALED_... to the expanded value of IMAGE_FSTYPES
> > 
> > base_contains() also expands the variable its passed.
> > 
> > I'm a little concerned as to why those two expansions of the same thing
> > should result in something different.
> 
> Well, they're happening at different parts in the parse.  The vmdk
> example is clearer I think since it's not another "big" python function.
> 
> > From your later explaination, it would appear the use of the override is
> > causing problems but I don't understand how immediate expansion would
> > work around that :/.
> > 
> > So before this merges, more investigation is needed as there is
> > something not adding up...
> 
> Well, I think the high level answer is that no, we aren't applying
> OVERRIDES at the point when we're parsing classes still rather than a
> specific task.  Adding an immediate eval at this level does force things
> and then we see what we're supposed to.  My gut feeling here is that if
> we poke deeply enough at base_contains we'll see that we aren't passing
> along the expand flag when we thought we were.

I wondered about that but I did already check and:

http://git.yoctoproject.org/cgit.cgi/poky/tree/bitbake/lib/bb/utils.py

def contains(variable, checkvalues, truevalue, falsevalue, d):
    val = d.getVar(variable, True)

so it is being expanded afaict...

Cheers,

Richard
Tom Rini - June 27, 2014, 3:17 p.m.
On Fri, Jun 27, 2014 at 03:54:30PM +0100, Richard Purdie wrote:
> On Fri, 2014-06-27 at 10:16 -0400, Tom Rini wrote:
> > On Fri, Jun 27, 2014 at 02:40:44PM +0100, Richard Purdie wrote:
> > > On Thu, 2014-06-26 at 08:27 -0400, Tom Rini wrote:
> > > > Incase we have overrides applied to IMAGE_FSTYPES we need to make sure
> > > > that we evaluate it fully before performing the live and vmdk logic
> > > > checks.
> > > > 
> > > > Signed-off-by: Tom Rini <tom.rini@gmail.com>
> > > > ---
> > > >  meta/classes/image.bbclass | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> > > > index 79de5a2..ccfa1b1 100644
> > > > --- a/meta/classes/image.bbclass
> > > > +++ b/meta/classes/image.bbclass
> > > > @@ -78,10 +78,11 @@ do_rootfs[vardeps] += "BAD_RECOMMENDATIONS NO_RECOMMENDATIONS"
> > > >  
> > > >  do_build[depends] += "virtual/kernel:do_deploy"
> > > >  
> > > > +EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> > > >  def build_live(d):
> > > > -    if base_contains("IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > > > -        d.setVar('NOISO', base_contains('IMAGE_FSTYPES', "iso", "0", "1", d))
> > > > -        d.setVar('NOHDD', base_contains('IMAGE_FSTYPES', "hddimg", "0", "1", d))
> > > > +    if base_contains("EVALED_IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
> > > > +        d.setVar('NOISO', base_contains('EVALED_IMAGE_FSTYPES', "iso", "0", "1", d))
> > > > +        d.setVar('NOHDD', base_contains('EVALED_IMAGE_FSTYPES', "hddimg", "0", "1", d))
> > > >          if d.getVar('NOISO', True) == "0" or d.getVar('NOHDD', True) == "0":
> > > >              return "image-live"
> > > >          return ""
> > > 
> > > The trouble is this doesn't add up. 
> > > 
> > > EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
> > > 
> > > basically sets EVALED_... to the expanded value of IMAGE_FSTYPES
> > > 
> > > base_contains() also expands the variable its passed.
> > > 
> > > I'm a little concerned as to why those two expansions of the same thing
> > > should result in something different.
> > 
> > Well, they're happening at different parts in the parse.  The vmdk
> > example is clearer I think since it's not another "big" python function.
> > 
> > > From your later explaination, it would appear the use of the override is
> > > causing problems but I don't understand how immediate expansion would
> > > work around that :/.
> > > 
> > > So before this merges, more investigation is needed as there is
> > > something not adding up...
> > 
> > Well, I think the high level answer is that no, we aren't applying
> > OVERRIDES at the point when we're parsing classes still rather than a
> > specific task.  Adding an immediate eval at this level does force things
> > and then we see what we're supposed to.  My gut feeling here is that if
> > we poke deeply enough at base_contains we'll see that we aren't passing
> > along the expand flag when we thought we were.
> 
> I wondered about that but I did already check and:
> 
> http://git.yoctoproject.org/cgit.cgi/poky/tree/bitbake/lib/bb/utils.py
> 
> def contains(variable, checkvalues, truevalue, falsevalue, d):
>     val = d.getVar(variable, True)
> 
> so it is being expanded afaict...

Yeah, but what does that expand back to?  My bitbake internals-fu is
weak, but:
$ git grep def\ getVar
lib/bb/command.py:    def getVariable(self, command, params):
lib/bb/data.py:def getVar(var, d, exp = 0):
lib/bb/data.py:def getVarFlag(var, flag, d):
lib/bb/data.py:def getVarFlags(var, d):
lib/bb/data_smart.py:    def getVar(self, var, expand=False, noweakdefault=False
lib/bb/data_smart.py:    def getVarFlag(self, var, flag, expand=False, noweakdef
lib/bb/data_smart.py:    def getVarFlags(self, var, expand = False, internalflag

So where do we map back to a 'getVar' that takes expand as arg 2 not arg
3?
Richard Purdie - June 28, 2014, 8:35 a.m.
On Fri, 2014-06-27 at 11:17 -0400, Tom Rini wrote:
> On Fri, Jun 27, 2014 at 03:54:30PM +0100, Richard Purdie wrote:
> > I wondered about that but I did already check and:
> > 
> > http://git.yoctoproject.org/cgit.cgi/poky/tree/bitbake/lib/bb/utils.py
> > 
> > def contains(variable, checkvalues, truevalue, falsevalue, d):
> >     val = d.getVar(variable, True)
> > 
> > so it is being expanded afaict...
> 
> Yeah, but what does that expand back to?  My bitbake internals-fu is
> weak, but:
> $ git grep def\ getVar
> lib/bb/command.py:    def getVariable(self, command, params):
> lib/bb/data.py:def getVar(var, d, exp = 0):
> lib/bb/data.py:def getVarFlag(var, flag, d):
> lib/bb/data.py:def getVarFlags(var, d):
> lib/bb/data_smart.py:    def getVar(self, var, expand=False, noweakdefault=False
> lib/bb/data_smart.py:    def getVarFlag(self, var, flag, expand=False, noweakdef
> lib/bb/data_smart.py:    def getVarFlags(self, var, expand = False, internalflag
> 
> So where do we map back to a 'getVar' that takes expand as arg 2 not arg
> 3?

Its calling:

lib/bb/data_smart.py:    def getVar(self, var, expand=False, noweakdefault=False

since d is a data store object which becomes "self", the first argument
and the other two are then passed in with expand = True.

The bb.data.getVar(d, var, expand) syntax is just clumsy and deprecated
when you can write the same thing as d.getVar(var, expand).

Cheers,

Richard

Patch

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 79de5a2..ccfa1b1 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -78,10 +78,11 @@  do_rootfs[vardeps] += "BAD_RECOMMENDATIONS NO_RECOMMENDATIONS"
 
 do_build[depends] += "virtual/kernel:do_deploy"
 
+EVALED_IMAGE_FSTYPES := "${IMAGE_FSTYPES}"
 def build_live(d):
-    if base_contains("IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
-        d.setVar('NOISO', base_contains('IMAGE_FSTYPES', "iso", "0", "1", d))
-        d.setVar('NOHDD', base_contains('IMAGE_FSTYPES', "hddimg", "0", "1", d))
+    if base_contains("EVALED_IMAGE_FSTYPES", "live", "live", "0", d) == "0": # live is not set but hob might set iso or hddimg
+        d.setVar('NOISO', base_contains('EVALED_IMAGE_FSTYPES', "iso", "0", "1", d))
+        d.setVar('NOHDD', base_contains('EVALED_IMAGE_FSTYPES', "hddimg", "0", "1", d))
         if d.getVar('NOISO', True) == "0" or d.getVar('NOHDD', True) == "0":
             return "image-live"
         return ""
@@ -90,7 +91,7 @@  def build_live(d):
 IMAGE_TYPE_live = "${@build_live(d)}"
 
 inherit ${IMAGE_TYPE_live}
-IMAGE_TYPE_vmdk = '${@base_contains("IMAGE_FSTYPES", "vmdk", "image-vmdk", "", d)}'
+IMAGE_TYPE_vmdk = '${@base_contains("EVALED_IMAGE_FSTYPES", "vmdk", "image-vmdk", "", d)}'
 inherit ${IMAGE_TYPE_vmdk}
 
 python () {