[PATCH 0/2] Yocto Compatible 2.0 support code

[PATCH 0/2] Yocto Compatible 2.0 support code

[Patrick Ohly]

As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
+ signature changes" mail thread, changes in a .bbappend cannot be
done unconditionally. Making _append and _remove depend on overrides
which get set based on DISTRO_FEATURES is one way of achieving this.

The oe.utils.optional_includes() helper function has not been
discussed before. It's an attempt to address concerns by developers
that having to write code for (potentially complex) condition checking
is error prone and hard to read.

It depends on the bitbake enhancement that allows including multiple
files at once.

Patrick Ohly (2):
  bitbake.conf: DISTRO_FEATURES as overrides
  utils.py: helper function for optional include files

 meta/conf/bitbake.conf | 17 ++++++++++++++++-
 meta/lib/oe/utils.py   | 29 +++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

base-commit: 49c255494c1d0704a1c8c428281c81541b05dc3e
-- 
git-series 0.9.1

[PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 +
signature changes", changes in .bbappend must depend on some explicit
configuration change, typically selecting a distro feature.

For _append and _remove, adding an override that is set only when the
corresponding entry is in DISTRO_FEATURES achieves that:

  DISTRO_FEATURES = " ... my-distro-feature ... "

  do_install_append_my-distro-feature () {
       ...
  }

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/conf/bitbake.conf | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 3ad905c..ca6501e 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded"
 #
 # This works for  functions as well, they are really just environment variables.
 # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration.
-OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
+OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
 OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES"
 CLASSOVERRIDE ?= "class-target"
 DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}"
@@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE"
 
 FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}"
 
+# Turns certain DISTRO_FEATURES into overrides of the same name
+# or (optionally) some other name. Ensures that these special
+# distro features remain set also for native and nativesdk
+# recipes, so that these overrides can also be used there.
+#
+# Beware that this part of OVERRIDES changes during parsing, so usage
+# of these overrides should be limited to .bb and .bbappend files,
+# because then DISTRO_FEATURES is final.
+DISTRO_FEATURES_OVERRIDES ??= ""
+DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
+Each entry is added to OVERRIDES with the <feature> name if <feature> is in DISTRO_FEATURES."
+DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
+DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
+DISTROFEATURESOVERRIDES = "${@ ''.join([':' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
+
 ##################################################################
 # Include the rest of the config files.
 ##################################################################
-- 
git-series 0.9.1

Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Patrick Ohly
> Sent: den 7 juni 2017 17:32
> To: openembedded-core@lists.openembedded.org
> Subject: [OE-core] [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as
> overrides
> 
> As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 +
> signature changes", changes in .bbappend must depend on some explicit
> configuration change, typically selecting a distro feature.
> 
> For _append and _remove, adding an override that is set only when the
> corresponding entry is in DISTRO_FEATURES achieves that:
> 
>   DISTRO_FEATURES = " ... my-distro-feature ... "
> 
>   do_install_append_my-distro-feature () {
>        ...
>   }
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  meta/conf/bitbake.conf | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> index 3ad905c..ca6501e 100644
> --- a/meta/conf/bitbake.conf
> +++ b/meta/conf/bitbake.conf
> @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded"
>  #
>  # This works for  functions as well, they are really just environment
> variables.
>  # Default OVERRIDES to make compilation fail fast in case of build
> system misconfiguration.
> -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-
> ${BUILD_OS}:pn-
> ${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevari
> able"
> +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-
> ${BUILD_OS}:pn-
> ${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:
> ${CLASSOVERRIDE}:forcevariable"
>  OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES"
>  CLASSOVERRIDE ?= "class-target"
>  DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}"
> @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE"
> 
>  FILESOVERRIDES =
> "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}"
> 
> +# Turns certain DISTRO_FEATURES into overrides of the same name
> +# or (optionally) some other name. Ensures that these special
> +# distro features remain set also for native and nativesdk
> +# recipes, so that these overrides can also be used there.
> +#
> +# Beware that this part of OVERRIDES changes during parsing, so usage
> +# of these overrides should be limited to .bb and .bbappend files,
> +# because then DISTRO_FEATURES is final.
> +DISTRO_FEATURES_OVERRIDES ??= ""
> +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature>
> entries. \
> +Each entry is added to OVERRIDES with the <feature> name if <feature>
> is in DISTRO_FEATURES."
> +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
> +DISTRO_FEATURES_FILTER_NATIVESDK_append = "
> ${DISTRO_FEATURES_OVERRIDES}"
> +DISTROFEATURESOVERRIDES = "${@ ''.join([':' + x for x in
> (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) &
> set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
> +
>  ##################################################################
>  # Include the rest of the config files.
>  ##################################################################
> --
> git-series 0.9.1

Rather than requiring that the wanted DISTRO_FEATURES that should be 
available as overrides are defined in DISTRO_FEATURES_OVERRIDES (which 
should not be confused with the similarly named DISTROFEATURESOVERRIDES 
variable that you also add...), why not add them all but with a prefix. 
I.e., similar to how package names are available as overrides prefixed 
with "pn-", how about all distro features are made available as 
overrides with a "df-" prefix?

//Peter

Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote:
> Rather than requiring that the wanted DISTRO_FEATURES that should be 
> available as overrides are defined in DISTRO_FEATURES_OVERRIDES (which 
> should not be confused with the similarly named DISTROFEATURESOVERRIDES 
> variable that you also add...),

I had thought about those names and in the end went ahead with the
similar names because the customizable one made sense to me and the
internal one is similar to the other entries in OVERRIDES.

>  why not add them all but with a prefix. 
> I.e., similar to how package names are available as overrides prefixed 
> with "pn-", how about all distro features are made available as 
> overrides with a "df-" prefix?

That would be fine with me.

I just have a few concerns:
      * How performance-sensitive is OVERRIDES? How can the impact of
        both approaches be benchmarked? The idea behind the configurable
        subset was to add only a few new overrides. We currently have
        almost 70 individual entries in DISTRO_FEATURES.
      * I've seen confusion about the pn- prefix. At least df- would be
        named appropriately (in contrast to PN, which is historic), but
        it's yet another convention that might not be immediately
        obvious. The same is true for selecting a subset with the same
        name as the feature, though.
      * Can distro features contain characters that are invalid in an
        override? _ and : would have to be avoided, for example by
        mapping them to -.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Richard Purdie]

On Thu, 2017-06-08 at 08:04 +0200, Patrick Ohly wrote:
> On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote:
> > 
> > Rather than requiring that the wanted DISTRO_FEATURES that should
> > be 
> > available as overrides are defined in DISTRO_FEATURES_OVERRIDES
> > (which 
> > should not be confused with the similarly named
> > DISTROFEATURESOVERRIDES 
> > variable that you also add...),
> I had thought about those names and in the end went ahead with the
> similar names because the customizable one made sense to me and the
> internal one is similar to the other entries in OVERRIDES.
> 
> > 
> >  why not add them all but with a prefix. 
> > I.e., similar to how package names are available as overrides
> > prefixed 
> > with "pn-", how about all distro features are made available as 
> > overrides with a "df-" prefix?
> That would be fine with me.
> 
> I just have a few concerns:
>       * How performance-sensitive is OVERRIDES? How can the impact of
>         both approaches be benchmarked? The idea behind the
> configurable
>         subset was to add only a few new overrides. We currently have
>         almost 70 individual entries in DISTRO_FEATURES.
>       * I've seen confusion about the pn- prefix. At least df- would
> be
>         named appropriately (in contrast to PN, which is historic),
> but
>         it's yet another convention that might not be immediately
>         obvious. The same is true for selecting a subset with the
> same
>         name as the feature, though.
>       * Can distro features contain characters that are invalid in an
>         override? _ and : would have to be avoided, for example by
>         mapping them to -.

My feelings are:

* We need to use a prefix. We've been burnt too many times in the past 
  when not using these. "df-" is fine, users will just have to deal 
  with it.

* We should filter the list of overrides to those which the distro 
  wants to nominate. I really don't want to encourage wider spread of 
  things than we need to, these need to be conscious decisions.

Cheers,

Richard

Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Peter Kjellerstedt]

> -----Original Message-----
> From: Richard Purdie [mailto:richard.purdie@linuxfoundation.org]
> Sent: den 8 juni 2017 12:45
> To: Patrick Ohly <patrick.ohly@intel.com>; Peter Kjellerstedt
> <peter.kjellerstedt@axis.com>
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as
> overrides
> 
> On Thu, 2017-06-08 at 08:04 +0200, Patrick Ohly wrote:
> > On Wed, 2017-06-07 at 16:11 +0000, Peter Kjellerstedt wrote:
> > >
> > > Rather than requiring that the wanted DISTRO_FEATURES that should
> > > be
> > > available as overrides are defined in DISTRO_FEATURES_OVERRIDES
> > > (which
> > > should not be confused with the similarly named
> > > DISTROFEATURESOVERRIDES
> > > variable that you also add...),
> > I had thought about those names and in the end went ahead with the
> > similar names because the customizable one made sense to me and the
> > internal one is similar to the other entries in OVERRIDES.
> >
> > >
> > >  why not add them all but with a prefix.
> > > I.e., similar to how package names are available as overrides
> > > prefixed
> > > with "pn-", how about all distro features are made available as
> > > overrides with a "df-" prefix?
> > That would be fine with me.
> >
> > I just have a few concerns:
> >       * How performance-sensitive is OVERRIDES? How can the impact of
> >         both approaches be benchmarked? The idea behind the
> > configurable
> >         subset was to add only a few new overrides. We currently have
> >         almost 70 individual entries in DISTRO_FEATURES.
> >       * I've seen confusion about the pn- prefix. At least df- would
> > be
> >         named appropriately (in contrast to PN, which is historic),
> > but
> >         it's yet another convention that might not be immediately
> >         obvious. The same is true for selecting a subset with the
> > same
> >         name as the feature, though.
> >       * Can distro features contain characters that are invalid in an
> >         override? _ and : would have to be avoided, for example by
> >         mapping them to -.
> 
> My feelings are:
> 
> * We need to use a prefix. We've been burnt too many times in the past
>   when not using these. "df-" is fine, users will just have to deal
>   with it.
> 
> * We should filter the list of overrides to those which the distro
>   wants to nominate. I really don't want to encourage wider spread of
>   things than we need to, these need to be conscious decisions.

Isn't there a risk that will be confusing? I.e., recipes that use, e.g., 
bb.utils.contains() to check if a distro feature is set will be affected 
as soon as the feature is added to DISTRO_FEATURES, but recipes that 
use the override will only be affected if the feature has also been 
added to some filter variable.

> Cheers,
> 
> Richard

//Peter

Re: [PATCH 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Thu, 2017-06-08 at 13:16 +0000, Peter Kjellerstedt wrote:
> > My feelings are:
> > 
> > * We need to use a prefix. We've been burnt too many times in the past
> >   when not using these. "df-" is fine, users will just have to deal
> >   with it.

Fine with me.

> > * We should filter the list of overrides to those which the distro
> >   wants to nominate. I really don't want to encourage wider spread of
> >   things than we need to, these need to be conscious decisions.
> 
> Isn't there a risk that will be confusing? I.e., recipes that use, e.g., 
> bb.utils.contains() to check if a distro feature is set will be affected 
> as soon as the feature is added to DISTRO_FEATURES, but recipes that 
> use the override will only be affected if the feature has also been 
> added to some filter variable.

I'm not sure. I guess it boils down to proper documentation.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

[PATCH 2/2] utils.py: helper function for optional include files

[Patrick Ohly]

By using oe.utils.optional_includes(), developers can simplify the
code which selects which additional include files need to be included
in a .bbappend.

In the simple case (one distro feature and one include file) the code
is not shorter, but the intent is clearer than corresponding code
using bb.utils.contains():

   require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }

More complex cases are also supported, in particular include files
that are required for one of several distro features or multiple
different include files.

To keep the common use case simple, DISTRO_FEATURES are checked by
default. Checking IMAGE_FEATURES might also be useful.

The DISTRO_FEATURES default and the intended usage make this more
suitable for OE-core than bitbake.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/lib/oe/utils.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/meta/lib/oe/utils.py b/meta/lib/oe/utils.py
index 330a5ff..a6c6199 100644
--- a/meta/lib/oe/utils.py
+++ b/meta/lib/oe/utils.py
@@ -126,6 +126,35 @@ def features_backfill(var,d):
     if addfeatures:
         d.appendVar(var, " " + " ".join(addfeatures))
 
+def optional_includes(d, mapping, key_var="DISTRO_FEATURES"):
+    """
+    This can be used to generate a list of files to include depending on
+    the distro features that are selected. key_var contains the features
+    that are set, mapping_var a space-separated set of <feature(s)>:<file(s)>
+    entries. Features and files are separated by comma. Each file on the
+    right-hand side is included in the result once if any of the features one
+    the left-hand side is set.
+
+    Example:
+       require ${@ oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")}
+
+    For DISTRO_FEATURES = "foo xyz" that will include four .inc files in the
+    order in which they are listed.
+    """
+    key = set((d.getVar(key_var) or "").split())
+    mapping = mapping.split()
+    includes = []
+    for entry in mapping:
+        parts = entry.split(":", 1)
+        if len(parts) != 2:
+            bb.fatal("%s must contain entries of the form <feature(s)>:<file(s)>, not %s" % (mapping_var, entry))
+        features, files = parts
+        for feature in features.split(","):
+            if feature in key:
+                for file in files.split(","):
+                    if file not in includes:
+                        includes.append(file)
+    return " ".join(includes)
 
 def packages_filter_out_system(d):
     """
-- 
git-series 0.9.1

Re: [PATCH 2/2] utils.py: helper function for optional include files

[Richard Purdie]

On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> By using oe.utils.optional_includes(), developers can simplify the
> code which selects which additional include files need to be included
> in a .bbappend.
> 
> In the simple case (one distro feature and one include file) the code
> is not shorter, but the intent is clearer than corresponding code
> using bb.utils.contains():
> 
>    require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }
> 
> More complex cases are also supported, in particular include files
> that are required for one of several distro features or multiple
> different include files.
> 
> To keep the common use case simple, DISTRO_FEATURES are checked by
> default. Checking IMAGE_FEATURES might also be useful.
> 
> The DISTRO_FEATURES default and the intended usage make this more
> suitable for OE-core than bitbake.

I'm honestly not sure this actually aids readability. Taking your
example:

require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")}

I think I actually prefer:

require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc")}
require ${@oe.utils.optional_includes(d, "xyz:x.inc,y.inc,z.inc")}

since its more explict. Spelling this out with the existing syntax
isn't so bad either, I probably still think that this is clearer too:

require ${@bb.utils.contains(d, "DISTRO_FEATURES", "foo bar", "foo-or-bar.inc", "")}
require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc y.inc z.inc", "")}

How about simply:

require ${@oe.utils.distro_features(d, "foo bar", "foo-or-bar.inc")}
require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")}

?

Cheers,

Richard

Re: [PATCH 2/2] utils.py: helper function for optional include files

[Patrick Ohly]

On Thu, 2017-06-08 at 10:20 +0100, Richard Purdie wrote:
> On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> > By using oe.utils.optional_includes(), developers can simplify the
> > code which selects which additional include files need to be included
> > in a .bbappend.
> > 
> > In the simple case (one distro feature and one include file) the code
> > is not shorter, but the intent is clearer than corresponding code
> > using bb.utils.contains():
> > 
> >    require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }
> > 
> > More complex cases are also supported, in particular include files
> > that are required for one of several distro features or multiple
> > different include files.
> > 
> > To keep the common use case simple, DISTRO_FEATURES are checked by
> > default. Checking IMAGE_FEATURES might also be useful.
> > 
> > The DISTRO_FEATURES default and the intended usage make this more
> > suitable for OE-core than bitbake.
> 
> I'm honestly not sure this actually aids readability.

Fair enough.

> Taking your example:
> 
> require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc xyz:x.inc,y.inc,z.inc")}
> 
> I think I actually prefer:
> 
> require ${@oe.utils.optional_includes(d, "foo,bar:foo-or-bar.inc")}
> require ${@oe.utils.optional_includes(d, "xyz:x.inc,y.inc,z.inc")}
> 
> since its more explict. Spelling this out with the existing syntax
> isn't so bad either, I probably still think that this is clearer too:
> 
> require ${@bb.utils.contains(d, "DISTRO_FEATURES", "foo bar", "foo-or-bar.inc", "")}

That's the "foo and bar" case, not "foo or bar". It's an actual mistake
that people have made and that didn't get caught during code review.

> require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc y.inc z.inc", "")}
> 
> How about simply:
> 
> require ${@oe.utils.distro_features(d, "foo bar", "foo-or-bar.inc")}
> require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")}

That works for me, I just wonder about the exact semantic of listing
multiple features: "and" as in contains(), or "any"?

I'm leaning towards two functions where that is explicit:
  oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc")
  oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc")

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH 2/2] utils.py: helper function for optional include files

[Richard Purdie]

On Thu, 2017-06-08 at 16:36 +0200, Patrick Ohly wrote:
> On Thu, 2017-06-08 at 10:20 +0100, Richard Purdie wrote:
> > 
> > require ${@bb.utils.contains(d, "DISTRO_FEATURES", "xyz", "x.inc
> > y.inc z.inc", "")}
> > 
> > How about simply:
> > 
> > require ${@oe.utils.distro_features(d, "foo bar", "foo-or-
> > bar.inc")}
> > require ${@bb.utils.distro_features(d, "xyz", "x.inc y.inc z.inc")}
> That works for me, I just wonder about the exact semantic of listing
> multiple features: "and" as in contains(), or "any"?
> 
> I'm leaning towards two functions where that is explicit:
>   oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc")
>   oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc")

I think you're right, this distinction is easily missed and I think its
an idea to highlight it as you propose...

Cheers,

Richard

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

[Joshua Watt]

On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
> + signature changes" mail thread, changes in a .bbappend cannot be
> done unconditionally. Making _append and _remove depend on overrides
> which get set based on DISTRO_FEATURES is one way of achieving this.
> 
> The oe.utils.optional_includes() helper function has not been
> discussed before. It's an attempt to address concerns by developers
> that having to write code for (potentially complex) condition
> checking
> is error prone and hard to read.

I promise I'm not trying to start a flame war here, and perhaps there
is history behind this that I'm not aware of but...

Why doesn't bitbake support some sort of "if" statement? It seems like
most of what we are trying to do could be accomplished with much less
fuss if one could simply do this in the bb file:

 if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d):
    include foo.inc

One could even eliminate the separate inc file and simply put its
contents under the conditional (as much fun as it seems to have to open
a new file just to see what a recipe is doing with a distro feature...)

It would also appear that this could make a lot of other things simpler
as well (and may even negate the need to backfill DISTRO_FEATURES into
overrides?)


> 
> It depends on the bitbake enhancement that allows including multiple
> files at once.
> 
> Patrick Ohly (2):
>   bitbake.conf: DISTRO_FEATURES as overrides
>   utils.py: helper function for optional include files
> 
>  meta/conf/bitbake.conf | 17 ++++++++++++++++-
>  meta/lib/oe/utils.py   | 29 +++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> base-commit: 49c255494c1d0704a1c8c428281c81541b05dc3e
> -- 
> git-series 0.9.1

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

[Richard Purdie]

On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote:
> On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> > 
> > As discussed in the "[Openembedded-architecture] Yocto Compatible
> > 2.0
> > + signature changes" mail thread, changes in a .bbappend cannot be
> > done unconditionally. Making _append and _remove depend on
> > overrides
> > which get set based on DISTRO_FEATURES is one way of achieving
> > this.
> > 
> > The oe.utils.optional_includes() helper function has not been
> > discussed before. It's an attempt to address concerns by developers
> > that having to write code for (potentially complex) condition
> > checking
> > is error prone and hard to read.
> I promise I'm not trying to start a flame war here, and perhaps there
> is history behind this that I'm not aware of but...
> 
> Why doesn't bitbake support some sort of "if" statement? It seems
> like most of what we are trying to do could be accomplished with much
> less fuss if one could simply do this in the bb file:
> 
>  if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d):
>     include foo.inc

This wouldn't actually solve as much of the problem as you think it
might at first glance and probably causes others, at least as I
understand it (as someone who's worked on bitbake's override code).

For example, at what point does this get evaluated? Most bitbake
variables are expanded at usage time, not parse time but here, the way
the parser works today, it would have to do an immediate expansion of
DISTRO_FEATURES to decide whether to include this file (or code block).

So ok, lets assume we change bitbake massively and defer the expansion
somehow. What if foo.inc influences the contents of DISTRO_FEATURES?
Should it then "unparse" foo.inc if my-feature was removed? or error?
or silently ignore that?

bitbake's main conditional today is through overrides and these do
allow a controlled delayed expansion of metadata in most cases. In some
cases such as include and inherit statements there is still the
immediate expansion issue above but at least there aren't huge changes
to the parser required to make it work so its the best of both worlds.

> One could even eliminate the separate inc file and simply put its
> contents under the conditional (as much fun as it seems to have to
> open
> a new file just to see what a recipe is doing with a distro
> feature...)
> 
> It would also appear that this could make a lot of other things
> simpler as well (and may even negate the need to backfill
> DISTRO_FEATURES into overrides?)

See if the above gives food for thought on that...

The big problems are the corner cases. If we do add new syntax it needs
to avoid these as we already have some pretty nasty ones, thankfully
most people don't hit them though.

Cheers,

Richard

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

[Joshua Watt]

On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote:
> On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote:
> > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> > > 
> > > As discussed in the "[Openembedded-architecture] Yocto Compatible
> > > 2.0
> > > + signature changes" mail thread, changes in a .bbappend cannot
> > > be
> > > done unconditionally. Making _append and _remove depend on
> > > overrides
> > > which get set based on DISTRO_FEATURES is one way of achieving
> > > this.
> > > 
> > > The oe.utils.optional_includes() helper function has not been
> > > discussed before. It's an attempt to address concerns by
> > > developers
> > > that having to write code for (potentially complex) condition
> > > checking
> > > is error prone and hard to read.
> > 
> > I promise I'm not trying to start a flame war here, and perhaps
> > there
> > is history behind this that I'm not aware of but...
> > 
> > Why doesn't bitbake support some sort of "if" statement? It seems
> > like most of what we are trying to do could be accomplished with
> > much
> > less fuss if one could simply do this in the bb file:
> > 
> >  if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d):
> >     include foo.inc
> 
> This wouldn't actually solve as much of the problem as you think it
> might at first glance and probably causes others, at least as I
> understand it (as someone who's worked on bitbake's override code).
> 
> For example, at what point does this get evaluated? Most bitbake
> variables are expanded at usage time, not parse time but here, the
> way
> the parser works today, it would have to do an immediate expansion of
> DISTRO_FEATURES to decide whether to include this file (or code
> block).

Doesn't this same argument apply to doing a conditional include of a
file? When bitbake goes to resolve the file name while evaluating the
AST, it has to evaluate DISTRO_FEATURES which might not be complete. If
the conditional in an "if" statement were also evaluated when
evaluating the AST, I believe the following snipet:

 require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }

Would be (functionally) identical to something (sort of) like:

 if oe.utils.optional_includes(d, 'foo-feature:True'):
     <All of the content of bar.inc>

Without requiring splitting the recipe content up into multiple files.


> So ok, lets assume we change bitbake massively and defer the
> expansion
> somehow. What if foo.inc influences the contents of DISTRO_FEATURES?
> Should it then "unparse" foo.inc if my-feature was removed? or error?
> or silently ignore that?
> 
> bitbake's main conditional today is through overrides and these do
> allow a controlled delayed expansion of metadata in most cases. In
> some
> cases such as include and inherit statements there is still the
> immediate expansion issue above but at least there aren't huge
> changes
> to the parser required to make it work so its the best of both
> worlds.

I was curious as to what it would it would actually take to make "if"
statements like the one I described above work (and I wanted to learn
more about the bitbake internals), so I did a proof of concept on
GitHub:
https://github.com/JPEWdev/poky/commit/998a00f122154bb509d22b412fba0773
97f6e433

It's actually not particularly terrible IMHO, but I'm sure it could be
better.

I can repost it to the bitbake mailing list as an RFC if you think that
would be helpful.

> 
> > One could even eliminate the separate inc file and simply put its
> > contents under the conditional (as much fun as it seems to have to
> > open
> > a new file just to see what a recipe is doing with a distro
> > feature...)
> > 
> > It would also appear that this could make a lot of other things
> > simpler as well (and may even negate the need to backfill
> > DISTRO_FEATURES into overrides?)
> 
> See if the above gives food for thought on that...
> 
> The big problems are the corner cases. If we do add new syntax it
> needs
> to avoid these as we already have some pretty nasty ones, thankfully
> most people don't hit them though.

That's fine. I"m not particularly trying to say that an "if" statement
is the magic bullet for corner cases, but I think it is equivalent
functionality to conditional includes and more readable and
maintainable for people writing recipes. Maybe that means
DISTRO_FEATURES still need to become OVERRIDES, IDK.

> 
> Cheers,
> 
> Richard

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

[Richard Purdie]

On Thu, 2017-06-08 at 08:55 -0500, Joshua Watt wrote:
> On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote:
> > 
> > On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote:
> > > 
> > > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> > > > 
> > > > 
> > > > As discussed in the "[Openembedded-architecture] Yocto
> > > > Compatible
> > > > 2.0
> > > > + signature changes" mail thread, changes in a .bbappend cannot
> > > > be
> > > > done unconditionally. Making _append and _remove depend on
> > > > overrides
> > > > which get set based on DISTRO_FEATURES is one way of achieving
> > > > this.
> > > > 
> > > > The oe.utils.optional_includes() helper function has not been
> > > > discussed before. It's an attempt to address concerns by
> > > > developers
> > > > that having to write code for (potentially complex) condition
> > > > checking
> > > > is error prone and hard to read.
> > > I promise I'm not trying to start a flame war here, and perhaps
> > > there
> > > is history behind this that I'm not aware of but...
> > > 
> > > Why doesn't bitbake support some sort of "if" statement? It seems
> > > like most of what we are trying to do could be accomplished with
> > > much
> > > less fuss if one could simply do this in the bb file:
> > > 
> > >  if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d):
> > >     include foo.inc
> > This wouldn't actually solve as much of the problem as you think it
> > might at first glance and probably causes others, at least as I
> > understand it (as someone who's worked on bitbake's override code).
> > 
> > For example, at what point does this get evaluated? Most bitbake
> > variables are expanded at usage time, not parse time but here, the
> > way
> > the parser works today, it would have to do an immediate expansion
> > of
> > DISTRO_FEATURES to decide whether to include this file (or code
> > block).
> Doesn't this same argument apply to doing a conditional include of a
> file? When bitbake goes to resolve the file name while evaluating the
> AST, it has to evaluate DISTRO_FEATURES which might not be complete.
> If
> the conditional in an "if" statement were also evaluated when
> evaluating the AST, I believe the following snipet:
> 
>  require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }
> 
> Would be (functionally) identical to something (sort of) like:
> 
>  if oe.utils.optional_includes(d, 'foo-feature:True'):
>      <All of the content of bar.inc>
> 
> Without requiring splitting the recipe content up into multiple
> files.

I did say the problem applied to the require syntax, yes.

Put another way, my big worry is that the if syntax will make people
start to want if syntax for things other than include style operations
and try and do other things other than "inclusion" type work with it.
We'll also need to then start dealing with nesting and most likely
other complications as well as pushing us to having to deal with
immediate expansion problems.

One of the strengths of the current syntax we have to day is that it
makes most things possible but does try and encourage you to do things
"the right way". In adding an if syntax like this I suspect we're on a
path which won't lead to a good place. I appreciate this isn't an exact
science answer :/.


To recap on how we get here, there is a problem of selective content
inclusion in distros/layers. Right now you tend to have to buy into
everything in a layer or nothing. This is bad for usability and
adoption of components in layers. Sometimes its not practical to
separate everything into isolated layers.

We've therefore tried to come up with a way of handling this adding
minimal changes but allowing the configuration we need.

We do need to try and limit the scope of the usage of this as there is
a fundamental issue, namely immediate expansion. I know most users will
not realise there is even a problem with this.

*If* we limit the scope to DISTRO_FEATURES, we stand a reasonable
chance of being able to limit the occasions a user runs into this.

On the other hand, if we add a generic if syntax, encourage usage of
any variable and so on I think we're setting ourselves up for failure.

Patrick for example mentioned IMAGE_FEATURES. This one is fraught with
problems since:

a) Its a recipe level setting so using it in a base configuration
context would end badly

b) Users change this in a variety of places some of which would be
bitten by the immediate expansion problem even just in recipe context

So no, I really don't like the idea of the if syntax, attractive as it
may look at first.

Cheers,

Richard

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

[Patrick Ohly]

On Thu, 2017-06-08 at 15:33 +0100, Richard Purdie wrote:
> Patrick for example mentioned IMAGE_FEATURES. This one is fraught with
> problems since:
> 
> a) Its a recipe level setting so using it in a base configuration
> context would end badly

We cannot even rely on DISTRO_FEATURES in the middle of the base
configuration construction; basically conditional includes in the base
configuration are not reliable at all.

> b) Users change this in a variety of places some of which would be
> bitten by the immediate expansion problem even just in recipe context

I agree. The same had already occurred to me, with a slightly different
failure mode:

c) base image recipe does the conditional include based on
   IMAGE_FEATURES, bbappend is used to change IMAGE_FEATURES => include
   done based on something other than the final IMAGE_FEATURES

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Joshua Watt]

On Thu, 2017-06-08 at 15:33 +0100, Richard Purdie wrote:
> On Thu, 2017-06-08 at 08:55 -0500, Joshua Watt wrote:
> > On Thu, 2017-06-08 at 09:56 +0100, Richard Purdie wrote:
> > > 
> > > On Wed, 2017-06-07 at 10:43 -0500, Joshua Watt wrote:
> > > > 
> > > > On Wed, 2017-06-07 at 17:31 +0200, Patrick Ohly wrote:
> > > > > 
> > > > > 
> > > > > As discussed in the "[Openembedded-architecture] Yocto
> > > > > Compatible
> > > > > 2.0
> > > > > + signature changes" mail thread, changes in a .bbappend
> > > > > cannot
> > > > > be
> > > > > done unconditionally. Making _append and _remove depend on
> > > > > overrides
> > > > > which get set based on DISTRO_FEATURES is one way of
> > > > > achieving
> > > > > this.
> > > > > 
> > > > > The oe.utils.optional_includes() helper function has not been
> > > > > discussed before. It's an attempt to address concerns by
> > > > > developers
> > > > > that having to write code for (potentially complex) condition
> > > > > checking
> > > > > is error prone and hard to read.
> > > > 
> > > > I promise I'm not trying to start a flame war here, and perhaps
> > > > there
> > > > is history behind this that I'm not aware of but...
> > > > 
> > > > Why doesn't bitbake support some sort of "if" statement? It
> > > > seems
> > > > like most of what we are trying to do could be accomplished
> > > > with
> > > > much
> > > > less fuss if one could simply do this in the bb file:
> > > > 
> > > >  if bb.utils.contains('DISTRO_FEATURES', 'my-feature', d):
> > > >     include foo.inc
> > > 
> > > This wouldn't actually solve as much of the problem as you think
> > > it
> > > might at first glance and probably causes others, at least as I
> > > understand it (as someone who's worked on bitbake's override
> > > code).
> > > 
> > > For example, at what point does this get evaluated? Most bitbake
> > > variables are expanded at usage time, not parse time but here,
> > > the
> > > way
> > > the parser works today, it would have to do an immediate
> > > expansion
> > > of
> > > DISTRO_FEATURES to decide whether to include this file (or code
> > > block).
> > 
> > Doesn't this same argument apply to doing a conditional include of
> > a
> > file? When bitbake goes to resolve the file name while evaluating
> > the
> > AST, it has to evaluate DISTRO_FEATURES which might not be
> > complete.
> > If
> > the conditional in an "if" statement were also evaluated when
> > evaluating the AST, I believe the following snipet:
> > 
> >  require ${@ oe.utils.optional_includes(d, 'foo-feature:bar.inc') }
> > 
> > Would be (functionally) identical to something (sort of) like:
> > 
> >  if oe.utils.optional_includes(d, 'foo-feature:True'):
> >      <All of the content of bar.inc>
> > 
> > Without requiring splitting the recipe content up into multiple
> > files.
> 
> I did say the problem applied to the require syntax, yes.
> 
> Put another way, my big worry is that the if syntax will make people
> start to want if syntax for things other than include style
> operations
> and try and do other things other than "inclusion" type work with it.
> We'll also need to then start dealing with nesting and most likely
> other complications as well as pushing us to having to deal with
> immediate expansion problems.
> 
> One of the strengths of the current syntax we have to day is that it
> makes most things possible but does try and encourage you to do
> things
> "the right way". In adding an if syntax like this I suspect we're on
> a
> path which won't lead to a good place. I appreciate this isn't an
> exact
> science answer :/.
> 
> 
> To recap on how we get here, there is a problem of selective content
> inclusion in distros/layers. Right now you tend to have to buy into
> everything in a layer or nothing. This is bad for usability and
> adoption of components in layers. Sometimes its not practical to
> separate everything into isolated layers.
> 
> We've therefore tried to come up with a way of handling this adding
> minimal changes but allowing the configuration we need.
> 
> We do need to try and limit the scope of the usage of this as there
> is
> a fundamental issue, namely immediate expansion. I know most users
> will
> not realise there is even a problem with this.
> 
> *If* we limit the scope to DISTRO_FEATURES, we stand a reasonable
> chance of being able to limit the occasions a user runs into this.
> 
> On the other hand, if we add a generic if syntax, encourage usage of
> any variable and so on I think we're setting ourselves up for
> failure.

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").

On the other hand, perhaps the range of terrible things that can be
done extends to more than just how you conditionally include something.
*What* is conditionally included might also require some scrutiny. As
you have alluded to, overrides are probably the best option for
variables, so putting them in a conditional include file is probably
not ideal. Forcing people to move the things that have to be
conditional to a separate file might actually be detrimental in a
number of ways:
 1) It might encourage recipe writers to do more in the include file
than they maybe should so that they don't have to make a plethora of
files.
 2) It might make it harder to verify that what the recipe writers did
is correct since the context of what they are doing is removed from the
parent recipe.

IIRC the conditional syntax (if or conditional include) is really
mostly needed for the parts of bitbake that don't allow overrides
(addtask and such). If that is the desired restriction, it would not be
difficult to have bitbake enforce that by only allowing the subset of
things that don't support overrides to be in the body of a if
statement. This would be more difficult with conditional includes
unless some other bitbake syntax was added.

> 
> Patrick for example mentioned IMAGE_FEATURES. This one is fraught
> with
> problems since:
> 
> a) Its a recipe level setting so using it in a base configuration
> context would end badly
> 
> b) Users change this in a variety of places some of which would be
> bitten by the immediate expansion problem even just in recipe context

Yes, that is an annoying problem. But it is neither made better or
worse by the decision to use if vs conditional include (since they have
the same semantics).

> 

> So no, I really don't like the idea of the if syntax, attractive as
> it
> may look at first.

If that's the consensus, than I'm fine with that. From my perspective,
conditional includes are just another (more difficult to use) form of
an "if" statement, and making it difficult to do things conditionally
doesn't necessarily make it better for anyone.



> 
> Cheers,
> 
> Richard
> 
> 

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

[Patrick Ohly]

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.

This would also be useful for "inherit". I remember struggling to
understand why certain image type classes kept getting inherited despite
changing IMAGE_FSTYPES - it turned out, that change had to be made
earlier.

That's neither an argument for nor against the "if" check - the same
could be done for that. Just something that occurred to me.

> On the other hand, perhaps the range of terrible things that can be
> done extends to more than just how you conditionally include something.
> *What* is conditionally included might also require some scrutiny. As
> you have alluded to, overrides are probably the best option for
> variables, so putting them in a conditional include file is probably
> not ideal. Forcing people to move the things that have to be
> conditional to a separate file might actually be detrimental in a
> number of ways:
>  1) It might encourage recipe writers to do more in the include file
> than they maybe should so that they don't have to make a plethora of
> files.
>  2) It might make it harder to verify that what the recipe writers did
> is correct since the context of what they are doing is removed from the
> parent recipe.
> 
> IIRC the conditional syntax (if or conditional include) is really
> mostly needed for the parts of bitbake that don't allow overrides
> (addtask and such). If that is the desired restriction, it would not be
> difficult to have bitbake enforce that by only allowing the subset of
> things that don't support overrides to be in the body of a if
> statement. This would be more difficult with conditional includes
> unless some other bitbake syntax was added.

There's some truth to that IMHO, but I'm uncertain whether it warrants
introducing entirely new syntax. In refkit, I only ran into one
particular case were an include file was necessary.

> If that's the consensus, than I'm fine with that. From my perspective,
> conditional includes are just another (more difficult to use) form of
> an "if" statement, and making it difficult to do things conditionally
> doesn't necessarily make it better for anyone.

Making it hard sends the message that it shouldn't be used lightly.
Documentation will have to make clear that conditional includes are the
last resort when everything else isn't usable.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Patrick Ohly]

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 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.

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?

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():


-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Joshua Watt]

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():
> 
> 

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

[Patrick Ohly]

On Fri, 2017-06-09 at 08:47 -0500, Joshua Watt wrote:
> 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).

When doing it as sanity.bbclass check it would have a short ID string.

> > 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?).

It's independent of Yocto Compatible 2.0. The image type inheritance
issue I mentioned is a basic usability problem, and this TARGET_SYS
dependency looks like a genuine bug to me.

> > 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?

Using LAYERDIR is one, although that particular example is probably
better caught by looking for that particular variable instead of
annotating all places where it gets used.

I'm unsure whether there are valid uses. I'm just worried that by the
time someone shows up with one, it will be too late to add a suppression
mechanism (as in "I've just updated from 2.3 to 2.4, how do I disable
this warning?").

>  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" :)

I would add such an explanation to the error message itself and warn
about using it, then hopefully no-one writes such stupid blog posts ;-}

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Patrick Ohly]

On Fri, 2017-06-09 at 16:11 +0200, Patrick Ohly wrote:
> On Fri, 2017-06-09 at 08:47 -0500, Joshua Watt wrote:
> > Is there a currently known valid use for such a thing?
> 
> Using LAYERDIR is one, although that particular example is probably
> better caught by looking for that particular variable instead of
> annotating all places where it gets used.
> 
> I'm unsure whether there are valid uses.

Perhaps this one is a valid one:

WARNING: .../libgcc-initial_6.3.bb:1: include/require/inherit "recipes-devtools/gcc/gcc-${PV}.inc" resulted in including "recipes-devtools/gcc/gcc-6.3.inc" while parsing. Variables effecting the parameter changed later such that "recipes-devtools/gcc/gcc-6.3.0.inc" would have been included at the end of the recipe.

PV changes, but that must have been anticipated as it clearly works as
intended.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Patrick Ohly]

On Fri, 2017-06-09 at 10:12 +0200, Patrick Ohly wrote:
> 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.

And Andre McCurdy also came to the same conclusion - see his
"[OE-core] [PATCH] bitbake.conf: fix ineffective include conf/target/${TARGET_SYS}.conf"
patch. However, that patch IMHO is incomplete, because it doesn't account for
${DISTRO}.conf changing the TARGET_VENDOR part of TARGET_SYS. I'll comment there, too.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Joshua Watt]

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.
> 
> This would also be useful for "inherit". I remember struggling to
> understand why certain image type classes kept getting inherited
> despite
> changing IMAGE_FSTYPES - it turned out, that change had to be made
> earlier.
> 
> That's neither an argument for nor against the "if" check - the same
> could be done for that. Just something that occurred to me.
> 
> > On the other hand, perhaps the range of terrible things that can be
> > done extends to more than just how you conditionally include
> > something.
> > *What* is conditionally included might also require some scrutiny.
> > As
> > you have alluded to, overrides are probably the best option for
> > variables, so putting them in a conditional include file is
> > probably
> > not ideal. Forcing people to move the things that have to be
> > conditional to a separate file might actually be detrimental in a
> > number of ways:
> >  1) It might encourage recipe writers to do more in the include
> > file
> > than they maybe should so that they don't have to make a plethora
> > of
> > files.
> >  2) It might make it harder to verify that what the recipe writers
> > did
> > is correct since the context of what they are doing is removed from
> > the
> > parent recipe.
> > 
> > IIRC the conditional syntax (if or conditional include) is really
> > mostly needed for the parts of bitbake that don't allow overrides
> > (addtask and such). If that is the desired restriction, it would
> > not be
> > difficult to have bitbake enforce that by only allowing the subset
> > of
> > things that don't support overrides to be in the body of a if
> > statement. This would be more difficult with conditional includes
> > unless some other bitbake syntax was added.
> 
> There's some truth to that IMHO, but I'm uncertain whether it
> warrants
> introducing entirely new syntax. In refkit, I only ran into one
> particular case were an include file was necessary.

I'd be curious to see that. How big was the .inc file?

> 
> > If that's the consensus, than I'm fine with that. From my
> > perspective,
> > conditional includes are just another (more difficult to use) form
> > of
> > an "if" statement, and making it difficult to do things
> > conditionally
> > doesn't necessarily make it better for anyone.
> 
> Making it hard sends the message that it shouldn't be used lightly.
> Documentation will have to make clear that conditional includes are
> the
> last resort when everything else isn't usable.
> 

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

[Patrick Ohly]

On Fri, 2017-06-09 at 08:50 -0500, Joshua Watt wrote:
> On Thu, 2017-06-08 at 21:31 +0200, Patrick Ohly wrote:
> > There's some truth to that IMHO, but I'm uncertain whether it
> > warrants
> > introducing entirely new syntax. In refkit, I only ran into one
> > particular case were an include file was necessary.
> 
> I'd be curious to see that. How big was the .inc file?

It's here:
https://github.com/intel/intel-iot-refkit/blob/master/meta-refkit-core/recipes-devtools/bmap-tools/classes/bmap-tools-deploy.bbclass

That's the code still using the "inherit" workaround.

Not particularly large, but does things like "inherit deploy" and
"addtask" which themselves then either require conditional inherit or
anonymous python workarounds, so overall having them in a conditional
file is cleaner.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

[PATCH v2 0/2] Yocto Compatible 2.0 support code

[Patrick Ohly]

As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
+ signature changes" mail thread, changes in a .bbappend cannot be
done unconditionally. Making _append and _remove depend on overrides
which get set based on DISTRO_FEATURES is one way of achieving this.

Conditional includes can have unexpected results when used with checks
that still change during parsing. Helper functions might make this a bit
safer and easier to review by limiting the check to DISTRO_FEATURES.

Changes in V2:
  - added df- prefix to distro feature overrides
  - replaced too complex optional_includes() with simpler
    all/any_distro_features()

Patrick Ohly (2):
  bitbake.conf: DISTRO_FEATURES as overrides
  utils.py: helper function for optional include files

 meta/conf/bitbake.conf | 17 ++++++++++++++++-
 meta/lib/oe/utils.py   | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

base-commit: 186882ca62bf683b93cd7a250963921b89ba071f
-- 
git-series 0.9.1

[PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 +
signature changes", changes in .bbappend must depend on some explicit
configuration change, typically selecting a distro feature.

For _append and _remove, adding an override that is set only when the
corresponding entry is in DISTRO_FEATURES achieves that:

In local.conf:
  DISTRO_FEATURES_append = " my-distro-feature"

In layer.conf:
  DISTRO_FEATURES_OVERRIDES += "my-distro-feature"

In a .bbappend:
  do_install_append_df-my-distro-feature () {
       ...
  }

The subset of DISTRO_FEATURES that are made available as overrides
must be configured explicitly because using them this way should
be a conscious decision.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/conf/bitbake.conf | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 3ad905c..fce1882 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded"
 #
 # This works for  functions as well, they are really just environment variables.
 # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration.
-OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
+OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
 OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES"
 CLASSOVERRIDE ?= "class-target"
 DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}"
@@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE"
 
 FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}"
 
+# Turns certain DISTRO_FEATURES into overrides of the same name
+# or (optionally) some other name. Ensures that these special
+# distro features remain set also for native and nativesdk
+# recipes, so that these overrides can also be used there.
+#
+# Beware that this part of OVERRIDES changes during parsing, so usage
+# of these overrides should be limited to .bb and .bbappend files,
+# because then DISTRO_FEATURES is final.
+DISTRO_FEATURES_OVERRIDES ??= ""
+DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
+Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES."
+DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
+DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
+DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
+
 ##################################################################
 # Include the rest of the config files.
 ##################################################################
-- 
git-series 0.9.1

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Denys Dmytriyenko]

This now breaks parsing my distro config on these lines:

ENABLE_SYSVINIT ?= "0"
DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}"


Here's the log:

ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py
Traceback (most recent call last):
  File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'):
                 except Exception as exc:
    >                raise ExpansionError(varname, s, exc) from exc
     
bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined

-- 
Denys


On Fri, Jun 09, 2017 at 03:04:03PM +0200, Patrick Ohly wrote:
> As discussed in "[Openembedded-architecture] Yocto Compatible 2.0 +
> signature changes", changes in .bbappend must depend on some explicit
> configuration change, typically selecting a distro feature.
> 
> For _append and _remove, adding an override that is set only when the
> corresponding entry is in DISTRO_FEATURES achieves that:
> 
> In local.conf:
>   DISTRO_FEATURES_append = " my-distro-feature"
> 
> In layer.conf:
>   DISTRO_FEATURES_OVERRIDES += "my-distro-feature"
> 
> In a .bbappend:
>   do_install_append_df-my-distro-feature () {
>        ...
>   }
> 
> The subset of DISTRO_FEATURES that are made available as overrides
> must be configured explicitly because using them this way should
> be a conscious decision.
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  meta/conf/bitbake.conf | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> index 3ad905c..fce1882 100644
> --- a/meta/conf/bitbake.conf
> +++ b/meta/conf/bitbake.conf
> @@ -713,7 +713,7 @@ DISTRO_NAME ??= "OpenEmbedded"
>  #
>  # This works for  functions as well, they are really just environment variables.
>  # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration.
> -OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
> +OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
>  OVERRIDES[vardepsexclude] = "MACHINEOVERRIDES"
>  CLASSOVERRIDE ?= "class-target"
>  DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}"
> @@ -722,6 +722,21 @@ MACHINEOVERRIDES[vardepsexclude] = "MACHINE"
>  
>  FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}"
>  
> +# Turns certain DISTRO_FEATURES into overrides of the same name
> +# or (optionally) some other name. Ensures that these special
> +# distro features remain set also for native and nativesdk
> +# recipes, so that these overrides can also be used there.
> +#
> +# Beware that this part of OVERRIDES changes during parsing, so usage
> +# of these overrides should be limited to .bb and .bbappend files,
> +# because then DISTRO_FEATURES is final.
> +DISTRO_FEATURES_OVERRIDES ??= ""
> +DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
> +Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES."
> +DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
> +DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
> +DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
> +
>  ##################################################################
>  # Include the rest of the config files.
>  ##################################################################
> -- 
> git-series 0.9.1
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote:
> This now breaks parsing my distro config on these lines:
> 
> ENABLE_SYSVINIT ?= "0"
> DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}"
> 
> 
> Here's the log:
> 
> ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py
> Traceback (most recent call last):
>   File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'):
>                  except Exception as exc:
>     >                raise ExpansionError(varname, s, exc) from exc
>      
> bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined

base_conditional() seems to come from utils.bbclass, which gets
inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this
DISTRO_FEATURES_append end up getting expanded before these classes are
fully parsed.

I'll need to discuss this with Richard tomorrow.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Denys Dmytriyenko]

On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote:
> On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote:
> > This now breaks parsing my distro config on these lines:
> > 
> > ENABLE_SYSVINIT ?= "0"
> > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}"
> > 
> > 
> > Here's the log:
> > 
> > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py
> > Traceback (most recent call last):
> >   File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'):
> >                  except Exception as exc:
> >     >                raise ExpansionError(varname, s, exc) from exc
> >      
> > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined
> 
> base_conditional() seems to come from utils.bbclass, which gets
> inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this
> DISTRO_FEATURES_append end up getting expanded before these classes are
> fully parsed.

FWIW, replacing it with oe.utils.conditional() doesn't help.


> I'll need to discuss this with Richard tomorrow.

Richard mentioned this on IRC, but no solution yet. Please keep me posted.

-- 
Denys

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Mon, 2017-06-12 at 19:23 -0400, Denys Dmytriyenko wrote:
> On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote:
> > On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote:
> > > This now breaks parsing my distro config on these lines:
> > > 
> > > ENABLE_SYSVINIT ?= "0"
> > > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}"
> > > 
> > > 
> > > Here's the log:
> > > 
> > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py
> > > Traceback (most recent call last):
> > >   File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'):
> > >                  except Exception as exc:
> > >     >                raise ExpansionError(varname, s, exc) from exc
> > >      
> > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined
> > 
> > base_conditional() seems to come from utils.bbclass, which gets
> > inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this
> > DISTRO_FEATURES_append end up getting expanded before these classes are
> > fully parsed.
> 
> FWIW, replacing it with oe.utils.conditional() doesn't help.

How about the following patch? It solves the problem for me.

It also addresses some unease that I had when Richard proposed the
distro overrides approach (cyclic dependency of "DISTRO_FEATURES depends
on OVERRIDES which depends on DISTRO_FEATURES") by setting the overrides
once for the DISTRO_FEATURES as they are set at the end of the base
configuration. That's more deterministic; previously OVERRIDES could
basically change randomly during base configuration parsing.

I couldn't give a good example for this cyclic dependency at the time.
Let me add that now.

This code has an obvious cyclic dependency, and it indeed fails as I had
expected:

DISTRO_FEATURES_append = " ${@ bb.utils.contains('DISTRO_FEATURES', 'foo', 'bar', '', d) }"
DISTRO_FEATURES_append = " foo"

=> bb.data_smart.ExpansionError: Failure expanding variable
DISTRO_FEATURES, expression was ${@ bb.utils.contains('DISTRO_FEATURES',
'foo', 'bar', '', d) } which triggered exception RuntimeError: maximum
recursion depth exceeded

In this code, the cyclic dependency is a bit more hidden, and depending
on when things get evaluated, one gets foo and bar in DISTRO_FEATURES
(for example, in "bitbake -e" and thus recipes):
DISTRO_FEATURES_OVERRIDES += "bar"
DISTRO_FEATURES_append = " bar"
DISTRO_FEATURES_append_df-bar = " foo"

That last example still works with the patch below, but I would argue
that because of the conceptual issue (DISTRO_FEATURES depending on
content of DISTRO_FEATURES) it cannot be expected to work.

Once things get more complicated, it just fails in other unexpected
ways:
DISTRO_FEATURES_OVERRIDES += "bar foo"
DISTRO_FEATURES_append = " bar"
DISTRO_FEATURES_append_df-bar = " foo"

This adds bar and foo to DISTRO_FEATURES, but only bar ends up in
OVERRIDES when using the patch below. It was working with code
that sets distro overrides dynamically. This might be seen as a drawback
of the approach below, but as it only affects code that (IMHO) isn't
valid, I'm not too worried about that.

Let me also point out that the approach below completely sidesteps
potential vardeps problems, because the dependency of OVERRIDES on
DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My expectation
is that OVERRIDES are part of the base hash and that thus tracking of
how it gets computed isn't necessary. It seems to work in practice for
me:

DISTRO_FEATURES_append = " systemd"
DISTRO_FEATURES_OVERRIDES += "bar"
DISTRO_FEATURES_append = " bar"
PACKAGECONFIG_append_pn-systemd_df-bar = " gcrypt"

Changing either DISTRO_FEATURES_OVERRIDES or the DISTRO_FEATURES_append
changes the signature of systemd:do_configure as expected.

We are currently having a problem with the yocto-compat-layer.py
signature check in refkit where it complains about:
        List of dependencies for variable DISTROOVERRIDES changed from
        '{'DISTRO'}' to '{'DISTRO_FEATURES', 'DISTRO',
        'DISTROFEATURES2OVERRIDES'}'

That's for a slightly older OE-core and the distrooverrides.bbclass
approach. I don't know yet why that is now failing (it was working for
OE-core 2.3), but my expectation is that the approach below would avoid
it.

diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
index 78926656d77..356aeba72ad 100644
--- a/meta/classes/base.bbclass
+++ b/meta/classes/base.bbclass
@@ -234,6 +234,11 @@ python base_eventhandler() {
         # Works with the line in layer.conf which changes PATH to point here
         setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS', d)
         setup_hosttools_dir(d.getVar('HOSTTOOLS_DIR'), 'HOSTTOOLS_NONFATAL', d, fatal=False)
+        # Base configuration complete, DISTRO_FEATURES finalized => convert selected features
+        # into overrides.
+        e.data.setVar('DISTROFEATURESOVERRIDES',
+                      ''.join([':df-' + x for x in sorted(set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set(d.getVar('DISTRO_FEATURES').split()))]))
+
 
     if isinstance(e, bb.event.BuildStarted):
         localdata = bb.data.createCopy(e.data)
diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index bc438cca826..6069130f488 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -727,15 +727,23 @@ FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDE
 # distro features remain set also for native and nativesdk
 # recipes, so that these overrides can also be used there.
 #
-# Beware that this part of OVERRIDES changes during parsing, so usage
-# of these overrides should be limited to .bb and .bbappend files,
-# because then DISTRO_FEATURES is final.
+# Beware that DISTRO_FEATURES change during parsing of the
+# base configuration. These overrides can be used when setting
+# variables in the base configuration, but variable expansion
+# will only have the desired effect in recipes.
+#
+# Even worse, DISTRO_FEATURES might not be even usable during base
+# configuration parsing. For example, embedded Python code with calls
+# to base_conditional() can fail because the function is not defined.
+# yet. Therefore these conditionals are set by base.bbclass only after
+# parsing the base configuration is complete.
 DISTRO_FEATURES_OVERRIDES ??= ""
 DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
 Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES."
 DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
 DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
-DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
+# Initially empty, set once when DISTRO_FEATURES is stable.
+DISTROFEATURESOVERRIDES = ""
 
 ##################################################################
 # Include the rest of the config files.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Richard Purdie]

On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote:
> Let me also point out that the approach below completely sidesteps
> potential vardeps problems, because the dependency of OVERRIDES on
> DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My
> expectation is that OVERRIDES are part of the base hash and that thus
> tracking of how it gets computed isn't necessary.

I'm not sure what the best approach for the ordering issue is. The
system is setup to dynamically adjust for OVERRIDES as they change.
Perhaps setting it at the end of parsing is better, I'm not sure.

Regardless, you're not quite right above when you say that OVERRIDES
are part of the basehash, they're not in most cases.

We actually need OVERRIDES to change without changing the basehash, for
example when you switch MACHINE, you don't want allarch packages to
rebuild (or tune shared packages with the same tune but a different
MACHINE).

For this reason, OVERRIDES are very much not part of the hash, we rely
on computed values.

Your patch has broken that in several places and I'm trying to untangle
an unfortunate series of changes locally to resolve this behaviour
correctly.

Cheers,

Richard

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote:
> Let me also point out that the approach below completely sidesteps
> potential vardeps problems, because the dependency of OVERRIDES on
> DISTRO_FEATURES_OVERRIDES and DISTRO_FEATURES is hidden. My expectation
> is that OVERRIDES are part of the base hash and that thus tracking of
> how it gets computed isn't necessary. It seems to work in practice for
> me:
> 
> DISTRO_FEATURES_append = " systemd"
> DISTRO_FEATURES_OVERRIDES += "bar"
> DISTRO_FEATURES_append = " bar"
> PACKAGECONFIG_append_pn-systemd_df-bar = " gcrypt"
> 
> Changing either DISTRO_FEATURES_OVERRIDES or the DISTRO_FEATURES_append
> changes the signature of systemd:do_configure as expected.
> 
> We are currently having a problem with the yocto-compat-layer.py
> signature check in refkit where it complains about:
>         List of dependencies for variable DISTROOVERRIDES changed from
>         '{'DISTRO'}' to '{'DISTRO_FEATURES', 'DISTRO',
>         'DISTROFEATURES2OVERRIDES'}'
> 
> That's for a slightly older OE-core and the distrooverrides.bbclass
> approach. I don't know yet why that is now failing (it was working for
> OE-core 2.3), but my expectation is that the approach below would avoid
> it.

I've not figured out why it started failing. Because it's using the
custom distrooverrides.bbclass approach I've not investigated further.

What I was able to test now is the
refkit_poky.TestRefkitPoky.test_refkit_conf_signature selftest in
combination with OE-core/bitbake master (it wasn't working until
recently because of the OEQA selftest changes that we hadn't adapted to
yet).

That test checks that activating the refkit distro configuration without
using any of the refkit distro features doesn't change signatures.
That's a slightly stronger guarantee than required by Yocto Compatible
2.0; it's useful because it makes it possible to add the refkit distro
configuration without causing side effects when the developer doesn't
use the features.

When using the current bitbake.conf DISTRO_FEATURES_OVERRIDES support
from OE-core master, that test fails because adding the refkit distro
configuration needs to extend that variable, which shows up as (picking
one example):
   zlib:do_package_qa: a124d530af2a0531e4c18780f9c75c15 -> 56da5c6a55996ec5a4817d1d4003e5ed
      bitbake-diffsigs --task zlib do_package_qa --signature a124d530af2a0531e4c18780f9c75c15 56da5c6a55996ec5a4817d1d4003e5ed
      basehash changed from 870d9161cf8d10dfae610cf55b54612b to 528a46f1120a679282ff8e55bf2791cb
      Variable DISTRO_FEATURES_OVERRIDES value changed from '' to '      refkit-config     refkit-firewall     refkit-computervision     refkit-gateway '

This does not occur with the patch below.

On IRC Richard corrected me and explained that it isn't OVERRIDES which
gets tracked. My systemd example was working because variable values get
recorded after applying overrides, so indirectly OVERRIDES and thus
DISTRO_FEATURES_OVERRIDES+DISTRO_FEATURES change signatures.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH v2 1/2] bitbake.conf: DISTRO_FEATURES as overrides

[Patrick Ohly]

On Tue, 2017-06-13 at 09:14 +0200, Patrick Ohly wrote:
> On Mon, 2017-06-12 at 19:23 -0400, Denys Dmytriyenko wrote:
> > On Mon, Jun 12, 2017 at 11:05:19PM +0200, Patrick Ohly wrote:
> > > On Mon, 2017-06-12 at 15:46 -0400, Denys Dmytriyenko wrote:
> > > > This now breaks parsing my distro config on these lines:
> > > > 
> > > > ENABLE_SYSVINIT ?= "0"
> > > > DISTRO_FEATURES_append = "${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}"
> > > > 
> > > > 
> > > > Here's the log:
> > > > 
> > > > ERROR: Unable to parse /OE/arago-master/sources/bitbake/lib/bb/data_smart.py
> > > > Traceback (most recent call last):
> > > >   File "/OE/arago-master/sources/bitbake/lib/bb/data_smart.py", line 426, in DataSmart.expandWithRefs(s='${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)}', varname='DISTRO_FEATURES_append'):
> > > >                  except Exception as exc:
> > > >     >                raise ExpansionError(varname, s, exc) from exc
> > > >      
> > > > bb.data_smart.ExpansionError: Failure expanding variable DISTRO_FEATURES_append, expression was ${@base_conditional("ENABLE_SYSVINIT", "1", "", " systemd", d)} which triggered exception NameError: name 'base_conditional' is not defined
> > > 
> > > base_conditional() seems to come from utils.bbclass, which gets
> > > inherited by base.bbclass. Looks like DISTRO_FEATURES and thus this
> > > DISTRO_FEATURES_append end up getting expanded before these classes are
> > > fully parsed.
> > 
> > FWIW, replacing it with oe.utils.conditional() doesn't help.
> 
> How about the following patch? It solves the problem for me.

Richard had concerns about rewriting OVERRIDES in an event handler,
therefore we agreed to use the .bbclass approach that I originally
started with. I'm still testing that in refkit (blocked by trying to
move towards bleeding edge OE-core master-next, on which my patch was
based), but it should be ready at least for OE-core master-next, so I'll
post it now.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

[PATCH 1/2] Revert "bitbake.conf: DISTRO_FEATURES as overrides"

[Patrick Ohly]

This reverts commit 3b3ae91a22d6f685e804df4f32cdeebe1bd6bd88.

It turned out that the code which expands DISTRO_FEATURES early during
base config parsing can fail because some entries in DISTRO_FEATURES
might call Python functions like base_conditional() from base.bbclass
which aren't defined yet.

A different solution will be needed.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/conf/bitbake.conf | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 5d5ddec..80baec8 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -713,28 +713,13 @@ DISTRO_NAME ??= "OpenEmbedded"
 #
 # This works for  functions as well, they are really just environment variables.
 # Default OVERRIDES to make compilation fail fast in case of build system misconfiguration.
-OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}${DISTROFEATURESOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
+OVERRIDES = "${TARGET_OS}:${TRANSLATED_TARGET_ARCH}:build-${BUILD_OS}:pn-${PN}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}:${CLASSOVERRIDE}:forcevariable"
 CLASSOVERRIDE ?= "class-target"
 DISTROOVERRIDES ?= "${@d.getVar('DISTRO') or ''}"
 MACHINEOVERRIDES ?= "${MACHINE}"
 
 FILESOVERRIDES = "${TRANSLATED_TARGET_ARCH}:${MACHINEOVERRIDES}:${DISTROOVERRIDES}"
 
-# Turns certain DISTRO_FEATURES into overrides of the same name
-# or (optionally) some other name. Ensures that these special
-# distro features remain set also for native and nativesdk
-# recipes, so that these overrides can also be used there.
-#
-# Beware that this part of OVERRIDES changes during parsing, so usage
-# of these overrides should be limited to .bb and .bbappend files,
-# because then DISTRO_FEATURES is final.
-DISTRO_FEATURES_OVERRIDES ??= ""
-DISTRO_FEATURES_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
-Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES."
-DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
-DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
-DISTROFEATURESOVERRIDES = "${@ ''.join([':df-' + x for x in (set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
-
 ##################################################################
 # Include the rest of the config files.
 ##################################################################

base-commit: 6073813151383121f9fbd3dab9f33033ddf21ee2
-- 
git-series 0.9.1

[PATCH 2/2] distrooverrides.bbclass: DISTRO_FEATURES as overrides

[Patrick Ohly]

This achieves the same goal as the same change to bitbake.conf itself,
but because the class gets added later as part expanding INHERIT, this
new approach is less likely to run into problems when DISTRO_FEATURES
contains complex code.

Another difference is that the class currently does not get inherited
by default and thus is completely absent from a build unless some
layer or include file adds it to INHERIT.

Compared to the earlier code in bitbake.conf and a similar class in
intel-iot-refkit, additional overrides now get sorted. This makes the
final OVERRIDES more deterministic.

The lessons learned about unintentionally depending on OVERRIDES are
documented in the class because such problems are more likely to show
up as unexpected signature differences when using this class.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/classes/distrooverrides.bbclass | 32 +++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+)
 create mode 100644 meta/classes/distrooverrides.bbclass

diff --git a/meta/classes/distrooverrides.bbclass b/meta/classes/distrooverrides.bbclass
new file mode 100644
index 0000000..9a42712
--- /dev/null
+++ b/meta/classes/distrooverrides.bbclass
@@ -0,0 +1,32 @@
+# Turns certain DISTRO_FEATURES into overrides with the same
+# name plus a df- prefix. Ensures that these special
+# distro features remain set also for native and nativesdk
+# recipes, so that these overrides can also be used there.
+#
+# This makes it simpler to write .bbappends that only change the
+# task signatures of the recipe if the change is really enabled,
+# for example with:
+#   do_install_append_df-my-feature () { ... }
+# where "my-feature" is a DISTRO_FEATURE.
+#
+# The class is meant to be used in a layer.conf or distro
+# .inc file with:
+# INHERIT += "distrooverrides"
+# DISTRO_FEATURES_OVERRIDES += "my-feature"
+#
+# Beware that this part of OVERRIDES changes during parsing, so usage
+# of these overrides should be limited to .bb and .bbappend files,
+# because then DISTRO_FEATURES is final.
+
+DISTRO_FEATURE_OVERRIDES ?= ""
+DISTRO_FEATURE_OVERRIDES[doc] = "A space-separated list of <feature> entries. \
+Each entry is added to OVERRIDES as df-<feature> if <feature> is in DISTRO_FEATURES."
+
+DISTRO_FEATURES_FILTER_NATIVE_append = " ${DISTRO_FEATURES_OVERRIDES}"
+DISTRO_FEATURES_FILTER_NATIVESDK_append = " ${DISTRO_FEATURES_OVERRIDES}"
+
+# If DISTRO_FEATURES_OVERRIDES or DISTRO_FEATURES show up in a task
+# signature because of this line, then the task dependency on
+# OVERRIDES itself should be fixed. Excluding these two variables
+# with DISTROOVERRIDES[vardepsexclude] would just work around the problem.
+DISTROOVERRIDES .= "${@ ''.join([':df-' + x for x in sorted(set(d.getVar('DISTRO_FEATURES_OVERRIDES').split()) & set((d.getVar('DISTRO_FEATURES') or '').split()))]) }"
-- 
git-series 0.9.1

[PATCH v2 2/2] utils.py: helper function for optional include files

[Patrick Ohly]

The main intention is to provide easy-to-use and read helper functions
for including files only when certain distro features are
set. Functionally they are the same as bb.utils.contains and
bb.utils.contains_any.

Distro features are part of the base configuration and thus safe to
use for conditional inclusion in recipes and bbappends, in contrast to
recipe variables which might still change during parsing. Therefore
the check is limited to DISTRO_FEATURES. This is the reason for having
this in OE-core instead of bitbake.

Default values are set so that no redundant parameters have to be
passed for conditional includes. As a secondary usage, the functions
can also be used in boolean checks.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/lib/oe/utils.py | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/meta/lib/oe/utils.py b/meta/lib/oe/utils.py
index 330a5ff..822d0cd 100644
--- a/meta/lib/oe/utils.py
+++ b/meta/lib/oe/utils.py
@@ -126,6 +126,46 @@ def features_backfill(var,d):
     if addfeatures:
         d.appendVar(var, " " + " ".join(addfeatures))
 
+def all_distro_features(d, features, truevalue="1", falsevalue=""):
+    """
+    Returns truevalue if *all* given features are set in DISTRO_FEATURES,
+    else falsevalue. The features can be given as single string or anything
+    that can be turned into a set.
+
+    This is a shorter, more flexible version of
+    bb.utils.contains("DISTRO_FEATURES", features, truevalue, falsevalue, d).
+
+    Without explicit true/false values it can be used directly where
+    Python expects a boolean:
+       if oe.utils.all_distro_features(d, "foo bar"):
+           bb.fatal("foo and bar are mutually exclusive DISTRO_FEATURES")
+
+    With just a truevalue, it can be used to include files that are meant to be
+    used only when requested via DISTRO_FEATURES:
+       require ${@ oe.utils.all_distro_features(d, "foo bar", "foo-and-bar.inc")
+    """
+    return bb.utils.contains("DISTRO_FEATURES", features, truevalue, falsevalue, d)
+
+def any_distro_features(d, features, truevalue="1", falsevalue=""):
+    """
+    Returns truevalue if at least *one* of the given features is set in DISTRO_FEATURES,
+    else falsevalue. The features can be given as single string or anything
+    that can be turned into a set.
+
+    This is a shorter, more flexible version of
+    bb.utils.contains_any("DISTRO_FEATURES", features, truevalue, falsevalue, d).
+
+    Without explicit true/false values it can be used directly where
+    Python expects a boolean:
+       if not oe.utils.any_distro_features(d, "foo bar"):
+           bb.fatal("foo, bar or both must be set in DISTRO_FEATURES")
+
+    With just a truevalue, it can be used to include files that are meant to be
+    used only when requested via DISTRO_FEATURES:
+       require ${@ oe.utils.any_distro_features(d, "foo bar", "foo-or-bar.inc")
+
+    """
+    return bb.utils.contains_any("DISTRO_FEATURES", features, truevalue, falsevalue, d)
 
 def packages_filter_out_system(d):
     """
-- 
git-series 0.9.1

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

[Denys Dmytriyenko]

On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote:
> As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
> + signature changes" mail thread, changes in a .bbappend cannot be
> done unconditionally. Making _append and _remove depend on overrides
> which get set based on DISTRO_FEATURES is one way of achieving this.

Just wanted to confirm that this feature is going to be optional, right?


> Conditional includes can have unexpected results when used with checks
> that still change during parsing. Helper functions might make this a bit
> safer and easier to review by limiting the check to DISTRO_FEATURES.
> 
> Changes in V2:
>   - added df- prefix to distro feature overrides
>   - replaced too complex optional_includes() with simpler
>     all/any_distro_features()
> 
> Patrick Ohly (2):
>   bitbake.conf: DISTRO_FEATURES as overrides
>   utils.py: helper function for optional include files
> 
>  meta/conf/bitbake.conf | 17 ++++++++++++++++-
>  meta/lib/oe/utils.py   | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 1 deletion(-)
> 
> base-commit: 186882ca62bf683b93cd7a250963921b89ba071f
> -- 
> git-series 0.9.1
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

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

[Patrick Ohly]

On Sun, 2017-06-11 at 14:47 -0400, Denys Dmytriyenko wrote:
> On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote:
> > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
> > + signature changes" mail thread, changes in a .bbappend cannot be
> > done unconditionally. Making _append and _remove depend on overrides
> > which get set based on DISTRO_FEATURES is one way of achieving this.
> 
> Just wanted to confirm that this feature is going to be optional, right?

To achieve "Yocto Compatible 2.0" status, something like this will be
needed. But not everyone needs (or wants) that, so those who don't are
free to not use it, and it is off by default.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

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

[Denys Dmytriyenko]

On Mon, Jun 12, 2017 at 08:22:45AM +0200, Patrick Ohly wrote:
> On Sun, 2017-06-11 at 14:47 -0400, Denys Dmytriyenko wrote:
> > On Fri, Jun 09, 2017 at 03:04:02PM +0200, Patrick Ohly wrote:
> > > As discussed in the "[Openembedded-architecture] Yocto Compatible 2.0
> > > + signature changes" mail thread, changes in a .bbappend cannot be
> > > done unconditionally. Making _append and _remove depend on overrides
> > > which get set based on DISTRO_FEATURES is one way of achieving this.
> > 
> > Just wanted to confirm that this feature is going to be optional, right?
> 
> To achieve "Yocto Compatible 2.0" status, something like this will be
> needed. But not everyone needs (or wants) that, so those who don't are
> free to not use it, and it is off by default.

Thanks for clarification.

-- 
Denys

✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2)

[Patchwork]

== Series Details ==

Series: "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev2)
Revision: 2
URL   : https://patchwork.openembedded.org/series/7163/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at 78041e20e4)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe

✗ patchtest: failure for "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3)

[Patchwork]

== Series Details ==

Series: "[v2] bitbake.conf: DISTRO_FEAT..." and 1 more (rev3)
Revision: 3
URL   : https://patchwork.openembedded.org/series/7163/
State : failure

== Summary ==


Thank you for submitting this patch series to OpenEmbedded Core. This is
an automated response. Several tests have been executed on the proposed
series by patchtest resulting in the following failures:



* Issue             Series does not apply on top of target branch [test_series_merge_on_head] 
  Suggested fix    Rebase your series on top of targeted branch
  Targeted branch  master (currently at 78041e20e4)



If you believe any of these test results are incorrect, please reply to the
mailing list (openembedded-core@lists.openembedded.org) raising your concerns.
Otherwise we would appreciate you correcting the issues and submitting a new
version of the patchset if applicable. Please ensure you add/increment the
version number when sending the new version (i.e. [PATCH] -> [PATCH v2] ->
[PATCH v3] -> ...).

---
Test framework: http://git.yoctoproject.org/cgit/cgit.cgi/patchtest
Test suite:     http://git.yoctoproject.org/cgit/cgit.cgi/patchtest-oe