Re: [PATCH 0/2] Yocto Compatible 2.0 support code

[Joshua Watt <jpewhacker@gmail.com>]

On Fri, 2017-06-09 at 10:12 +0200, Patrick Ohly wrote:
> On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote:
> > On Thu, 2017-06-08 at 10:28 -0500, Joshua Watt wrote:
> > > Sure. I wouldn't suggest using an if statement for "just
> > > anything", you
> > > can surely do terrible things that way. It would (by convention)
> > > be
> > > restricted to the same sorts of things that the conditional
> > > includes
> > > allow now. On a similar token, you can do the same sorts of
> > > terrible
> > > things with conditional includes as currently proposed because it
> > > has
> > > the same enforcement policy (i.e. "by convention").
> > 
> > I'm starting to wonder whether this "convention" can be
> > strengthened
> > with additional warnings. The code which currently evaluates the
> > include
> > parameter could record in the datastore the original expression and
> > what
> > it evaluated to, then later when the recipe gets finalized, an
> > event
> > handler can check whether evaluating the expression still gives the
> > same
> > result.
> 
> Below is a quick-and-dirty proof of concept. Example bmap-tools_
> %.bbappend:
> 
>         FOO = "bmap-tools-deploy.inc"
>         require ${FOO}
>         FOO = ""
> 
> Example warning:
> 
> WARNING: .../bmap-tools/bmap-tools_3.2.bb: .../bmap-
> tools_%.bbappend:2: include/require/inherit "${FOO}" resulted in
> including "bmap-tools-deploy.inc" while parsing. Variables effecting
> the parameter changed later such that nothing would have been
> included at the end of the recipe.

I like it. The error message could maybe use a little bit of work...
Maybe it needs some sort of short and unique key words in it, like
"conditional invariance violation" to help direct people who copy the
error into google when looking for an answer (or slightly better, grep
the Mega Manual).

> 
> I found one false positive (LAYERDIR is set during parsing and unset
> afterwards) which the code below filters out. 
> 
> I also get for all recipes (i.e. the error is in the base
> configuration):
> 
> meta/conf/bitbake.conf:752: include/require/inherit
> "conf/target/${TARGET_SYS}.conf" resulted in including
> "conf/target/x86_64-oe-linux.conf" while parsing. Variables effecting
> the parameter changed later such that "conf/target/x86_64-refkit-
> linux.conf" would have been included at the end of the recipe.
> 
> None of these two files exist, so it doesn't make a difference. But
> is
> it really intended that a conf/target/${TARGET_SYS}.conf gets
> included
> that isn't the one for the final TARGET_SYS? That looks like a
> genuine
> bug to me.
> 
> TARGET_SYS changes because TARGET_VENDOR gets set by the
> ${DISTRO}.conf,
> which gets included later.
> 
> If we agree that such a message is useful, should it get added after
> merging the proposed bitbake enhancements or together with them?
> Doing
> it properly implies adding tests, and I do not have the time for that
> now. I'd prefer to add the proposed patches first so that they can be
> used, then add the warning before 2.4 gets released.
> 
> Should this warning be entirely in bitbake or should it become part
> of
> OE's sanity.bbclass? The latter would have the advantage that it
> could
> be configured as fatal error. The downside is that bitbake needs to
> export data, which implies adding a semi-stable API.

A fatal error would be nice.... anything that was 2.0 compatible should
be fatal, but I don't know if you would want it to make it fatal for
non-2.0 compatible (i.e. existing) layers, as it might break some
existing uses (or maybe we don't care about that?).


> 
> Do we want some suppression mechanism? Something like:
> 
> require ${PARSE_TIME_EVALUATION}${FOO}
> 
> where PARSE_TIME_EVALUATION is unset, but can be checked for in the
> code
> that normally would print the warning?

Is there a currently known valid use for such a thing? I'd just as soon
add it once we know it is needed (otherwise, the top search result for
"conditional invariance violation" will end up being some blog that
says "Just prefix it with ${PARSE_TIME_EVALUATION} and everything will
be fine" :)


> 
> diff --git a/lib/bb/parse/ast.py b/lib/bb/parse/ast.py
> index dba4540f5eb..91db56197f0 100644
> --- a/lib/bb/parse/ast.py
> +++ b/lib/bb/parse/ast.py
> @@ -50,14 +50,13 @@ class IncludeNode(AstNode):
>          """
>          Include the file and evaluate the statements
>          """
> -        s = data.expand(self.what_file)
> -        logger.debug(2, "CONF %s:%s: including %s", self.filename,
> self.lineno, s)
> +        logger.debug(2, "CONF %s:%s: including %s = %s",
> self.filename, self.lineno, self.what_file,
> data.expand(self.what_file))
>  
>          # TODO: Cache those includes... maybe not here though
>          if self.force:
> -            bb.parse.ConfHandler.include(self.filename, s,
> self.lineno, data, "include required")
> +            bb.parse.ConfHandler.include(self.filename,
> self.what_file, self.lineno, data, "include required")
>          else:
> -            bb.parse.ConfHandler.include(self.filename, s,
> self.lineno, data, False)
> +            bb.parse.ConfHandler.include(self.filename,
> self.what_file, self.lineno, data, False)
>  
>  class ExportNode(AstNode):
>      def __init__(self, filename, lineno, var):
> @@ -362,6 +361,18 @@ def finalize(fn, d, variant = None):
>  
>      d.setVar('BBINCLUDED', bb.parse.get_file_depends(d))
>  
> +    parse_expansions = d.getVar('_PARSE_TIME_EXPANSIONS')
> +    if parse_expansions:
> +        def fns_name(fns):
> +            if fns:
> +                return '"%s"' % fns
> +            else:
> +                return 'nothing'
> +        for parentfn, lineno, fns_unexpanded, fns in
> parse_expansions:
> +            current_fns = d.expand(fns_unexpanded)
> +            if fns != current_fns and '${LAYERDIR}' not in
> current_fns:
> +                logger.warning('%s:%d: include/require/inherit "%s"
> resulted in including %s while parsing. Variables effecting the
> parameter changed later such that %s would have been included at the
> end of the recipe.' % (parentfn, lineno, fns_unexpanded,
> fns_name(fns), fns_name(current_fns)))
> +
>      bb.event.fire(bb.event.RecipeParsed(fn), d)
>      bb.event.set_handlers(saved_handlers)
>  
> diff --git a/lib/bb/parse/parse_py/BBHandler.py
> b/lib/bb/parse/parse_py/BBHandler.py
> index fe918a41f34..923d210a3e8 100644
> --- a/lib/bb/parse/parse_py/BBHandler.py
> +++ b/lib/bb/parse/parse_py/BBHandler.py
> @@ -61,6 +61,7 @@ def supports(fn, d):
>  def inherit(files, fn, lineno, d):
>      __inherit_cache = d.getVar('__inherit_cache', False) or []
>      files = d.expand(files).split()
> +    # TODO: remember expansion result
>      for file in files:
>          if not os.path.isabs(file) and not
> file.endswith(".bbclass"):
>              file = os.path.join('classes', '%s.bbclass' % file)
> diff --git a/lib/bb/parse/parse_py/ConfHandler.py
> b/lib/bb/parse/parse_py/ConfHandler.py
> index 97aa1304314..b4f0c61d256 100644
> --- a/lib/bb/parse/parse_py/ConfHandler.py
> +++ b/lib/bb/parse/parse_py/ConfHandler.py
> @@ -75,8 +75,16 @@ def include(parentfn, fns, lineno, data,
> error_out):
>      used in a ParseError that will be raised if the file to be
> included could
>      not be included. Specify False to avoid raising an error in this
> case.
>      """
> +    fns_unexpanded = fns
>      fns = data.expand(fns)
>      parentfn = data.expand(parentfn)
> +    parse_expansions = data.getVar('_PARSE_TIME_EXPANSIONS')
> +    if parse_expansions is None:
> +        parse_expansions = []
> +    parse_expansions.append((parentfn, lineno, fns_unexpanded, fns))
> +    data.setVar('_PARSE_TIME_EXPANSIONS', parse_expansions)
> +    if 'bmap-tools' in parentfn:
> +        bb.note('bmap-tools parse_expansions: %s/%s = %s' %
> (id(data), id(parse_expansions), parse_expansions))
>  
>      # "include" or "require" accept zero to n space-separated file
> names to include.
>      for fn in fns.split():
> 
>