[PATCH] RFC: features.bbclass,bitbake.conf: use features lists

[PATCH] RFC: features.bbclass,bitbake.conf: use features lists

[Christopher Larson]

Any thoughts on something like this? Too much?

The intention is that downstreams would use standard bitbake functions
to check features, the same way they do in the Yocto Project, which
ensures that signatures and variable checksums never include the
entirety of the features variable, only the presence or absence of the
feature being checked.

This also adds bits to allow one to disable a feature without having
to use the problematic :remove mechanism, which can't be undone by
downstream layers easily. In this case, MY_FEATURES += "feature-a" and
then MY_FEATURES += "-feature-a" would result in removing it. Order
matters, so it can be re-added again later on without complication. It
also adds a minimal mechanism to let one feature pull in others
implicitly, which helps to avoid duplication when a feature is a
superset of another.
---

diff --git a/meta/classes/features.bbclass b/meta/classes/features.bbclass
new file mode 100644
index 00000000..32964c91
--- /dev/null
+++ b/meta/classes/features.bbclass
@@ -0,0 +1,91 @@
+# Functions to manipulate feature lists
+#
+# This class provides functions to manipulate feature lists. The functions
+# are intended to be used in :remove and :append handlers to remove or append
+# items to a variable. The remove will interpret items prefixed with a '-'
+# as items to be removed. The append function will rely on a mapping of
+# implied features to append the corresponding items. The intention is to
+# provide the ability for a feature to imply enabling of other features,
+# much like a dependency, but without the implication of order.
+#
+# Usage example:
+#
+#  IMAGE_FEATURES ??= ""
+#  IMAGE_FEATURES:remove = "${@remove_prefixed_features('IMAGE_FEATURES', d)}"
+#  IMAGE_FEATURES:prepend =
"${@add_implied_features('IMAGE_FEATURES', 'IMPLIED_IMAGE_FEATURES',
d)} "
+#  IMAGE_FEATURES_DISABLED = "${@' '.join(f for f in
remove_prefixed_features('IMAGE_FEATURES', d).split() if not
f.startswith('-'))}"
+#
+#  IMPLIED_IMAGE_FEATURES[alpha] = "beta"
+#
+#  IMAGE_FEATURES += "alpha theta -theta"
+#
+#  # SOME_VARIBLE="yes" if 'beta' is in IMAGE_FEATURES, "no" otherwise
+#  SOME_VARIABLE = "${@bb.utils.contains('IMAGE_FEATURES', 'beta',
'yes', 'no', d)}"
+#  # SOME_VARIBLE_TWO="yes" if 'beta' and 'alpha' are in
IMAGE_FEATURES, "no" otherwise
+#  SOME_VARIABLE_TWO = "${@bb.utils.contains('IMAGE_FEATURES',
['beta', 'alpha'], 'yes', 'no', d)}"
+#  # SOME_VARIBLE_THREE="yes" if 'beta' or 'theta' are in
IMAGE_FEATURES, "no" otherwise
+#  SOME_VARIABLE_THREE = "${@bb.utils.contains_any('IMAGE_FEATURES',
['beta', 'theta'], 'yes', 'no', d)}"
+
+
+def remove_prefixed_features(var, d):
+    """Return the items to be removed from var with :remove.
+
+    This function is intended to be used in a :remove handler to remove
+    items from a variable. It will interpret items prefixed with a '-' as
+    items to be removed.
+
+    As an internal note, the 'remove_prefixed_features_internal' flag
is used to
+    avoid infinite recursion.
+    """
+    if d.getVarFlag(var, 'remove_prefixed_features_internal') == '1':
+        return ''
+
+    from collections import Counter
+
+    d.setVarFlag(var, 'remove_prefixed_features_internal', '1')
+    try:
+        value = d.getVar(var)
+        counter = Counter()
+        for v in value.split():
+            if v.startswith('-'):
+                counter[v[1:]] -= 1
+                counter[v] -= 1
+            else:
+                counter[v] += 1
+        return ' '.join(v for v, c in counter.items() if c < 1)
+    finally:
+        d.delVarFlag(var, 'remove_prefixed_features_internal')
+
+def add_implied_features(var, implied_var, d):
+    """Return the items to be appended due to the presence of other
items in var.
+
+    This function is intended to be used in a :append handler to append
+    items from a variable. It will rely on the mapping of implied features
+    to append the corresponding items.
+
+    As an internal note, the 'add_implied_features_internal' flag is used to
+    avoid infinite recursion.
+    """
+    if d.getVarFlag(var, 'add_implied_features_internal') == '1':
+        return ''
+
+    def implied_features(feature, implied_mapping, d, seen=None):
+        """Return the implied features for a given feature."""
+        if seen is None:
+            seen = set()
+        if feature in seen:
+            return ''
+        seen.add(feature)
+        implied = implied_mapping.get(feature, '').split()
+        return ' '.join(implied + [implied_features(f,
implied_mapping, d, seen) for f in implied])
+
+    d.setVarFlag(var, 'add_implied_features_internal', '1')
+    try:
+        value = d.getVar(var)
+        implied_mapping = d.getVarFlags(implied_var)
+        if implied_mapping is None:
+            return ''
+
+        return ' '.join(implied_features(f, implied_mapping, d) for f
in value.split())
+    finally:
+        d.delVarFlag(var, 'add_implied_features_internal')
diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index cda98035..4dedb081 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -172,6 +172,31 @@ BBINCLUDELOGS ??= "yes"
 # Add event handlers for bitbake
 INHERIT += "isar-events sstate"

+# Make features variables available
+INHERIT += "features"
+
+BASE_REPO_FEATURES ??= ""
+BASE_REPO_FEATURES[doc] = "Specifies the list of features for the
base-apt repository."
+BASE_REPO_FEATURES:remove =
"${@remove_prefixed_features('BASE_REPO_FEATURES', d)}"
+BASE_REPO_FEATURES:prepend =
"${@add_implied_features('BASE_REPO_FEATURES',
'IMPLIED_BASE_REPO_FEATURES', d)} "
+
+MACHINE_FEATURES ??= ""
+MACHINE_FEATURES[doc] = "Specifies the list of hardware features the
MACHINE is capable of supporting."
+MACHINE_FEATURES:remove = "${@remove_prefixed_features('MACHINE_FEATURES', d)}"
+MACHINE_FEATURES:prepend =
"${@add_implied_features('MACHINE_FEATURES',
'IMPLIED_MACHINE_FEATURES', d)} "
+
+DISTRO_FEATURES ??= ""
+DISTRO_FEATURES[doc] = "The software support you want in your
distribution for various features."
+DISTRO_FEATURES:remove = "${@remove_prefixed_features('DISTRO_FEATURES', d)}"
+DISTRO_FEATURES:prepend = "${@add_implied_features('DISTRO_FEATURES',
'IMPLIED_DISTRO_FEATURES', d)} "
+
+COMBINED_FEATURES = "${@oe.utils.set_intersect('DISTRO_FEATURES',
'MACHINE_FEATURES', d)}"
+
+ROOTFS_FEATURES ??= ""
+ROOTFS_FEATURES[doc] = "The list of features to be included in a root
filesystem. Typically, you configure this variable in an image recipe
or class."
+ROOTFS_FEATURES:remove = "${@remove_prefixed_features('ROOTFS_FEATURES', d)}"
+ROOTFS_FEATURES:prepend = "${@add_implied_features('ROOTFS_FEATURES',
'IMPLIED_ROOTFS_FEATURES', d)} "
+
 # Buildstats requires IMAGE_ROOTFS to be always defined
 IMAGE_ROOTFS ??= "${WORKDIR}/rootfs"
 INHERIT += "${@'buildstats' if
bb.utils.to_boolean(d.getVar('USE_BUILDSTATS')) else ''}"
--
2.45.2

-- 
You received this message because you are subscribed to the Google Groups "isar-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/isar-users/CABcZANmMSpjZGqC4m%3DgVPQ-0QH88zVuXJmmvxpTx6brTvTfLDA%40mail.gmail.com.

Re: [PATCH] RFC: features.bbclass,bitbake.conf: use features lists

['Jan Kiszka' via isar-users]

On 11.11.24 17:51, Christopher Larson wrote:
> Any thoughts on something like this? Too much?
> 
> The intention is that downstreams would use standard bitbake functions
> to check features, the same way they do in the Yocto Project, which
> ensures that signatures and variable checksums never include the
> entirety of the features variable, only the presence or absence of the
> feature being checked.
> 
> This also adds bits to allow one to disable a feature without having
> to use the problematic :remove mechanism, which can't be undone by
> downstream layers easily. In this case, MY_FEATURES += "feature-a" and
> then MY_FEATURES += "-feature-a" would result in removing it. Order

Is that "-feature" feature something upstream OE has as well? Or how do
they handle that use case?

> matters, so it can be re-added again later on without complication. It
> also adds a minimal mechanism to let one feature pull in others
> implicitly, which helps to avoid duplication when a feature is a
> superset of another.

Same question: Would that syntax or even the logic isar-specific or in
line with OE?

BTW, missing signed-off, but I that might be because of "RFC".

> ---
> 
> diff --git a/meta/classes/features.bbclass b/meta/classes/features.bbclass
> new file mode 100644
> index 00000000..32964c91
> --- /dev/null
> +++ b/meta/classes/features.bbclass
> @@ -0,0 +1,91 @@

Missing license/copyright header.

> +# Functions to manipulate feature lists
> +#
> +# This class provides functions to manipulate feature lists. The functions
> +# are intended to be used in :remove and :append handlers to remove or append
> +# items to a variable. The remove will interpret items prefixed with a '-'
> +# as items to be removed. The append function will rely on a mapping of
> +# implied features to append the corresponding items. The intention is to
> +# provide the ability for a feature to imply enabling of other features,
> +# much like a dependency, but without the implication of order.
> +#
> +# Usage example:
> +#
> +#  IMAGE_FEATURES ??= ""
> +#  IMAGE_FEATURES:remove = "${@remove_prefixed_features('IMAGE_FEATURES', d)}"
> +#  IMAGE_FEATURES:prepend =
> "${@add_implied_features('IMAGE_FEATURES', 'IMPLIED_IMAGE_FEATURES',
> d)} "
> +#  IMAGE_FEATURES_DISABLED = "${@' '.join(f for f in
> remove_prefixed_features('IMAGE_FEATURES', d).split() if not
> f.startswith('-'))}"

What's the purpose of the last line? Or is it a stale example?

And then these three or four lines are kind of boilerplate for every
feature group one would like to create? Would there be a way to template
that with the feature variable name as parameter?

BTW, patch was mangled by your email client.

> +#
> +#  IMPLIED_IMAGE_FEATURES[alpha] = "beta"

So, if I add "alpha" to IMAGE_FEATURES, I will also get "beta" as
feature, right?

> +#
> +#  IMAGE_FEATURES += "alpha theta -theta"
> +#
> +#  # SOME_VARIBLE="yes" if 'beta' is in IMAGE_FEATURES, "no" otherwise
> +#  SOME_VARIABLE = "${@bb.utils.contains('IMAGE_FEATURES', 'beta',
> 'yes', 'no', d)}"
> +#  # SOME_VARIBLE_TWO="yes" if 'beta' and 'alpha' are in
> IMAGE_FEATURES, "no" otherwise
> +#  SOME_VARIABLE_TWO = "${@bb.utils.contains('IMAGE_FEATURES',
> ['beta', 'alpha'], 'yes', 'no', d)}"
> +#  # SOME_VARIBLE_THREE="yes" if 'beta' or 'theta' are in
> IMAGE_FEATURES, "no" otherwise
> +#  SOME_VARIABLE_THREE = "${@bb.utils.contains_any('IMAGE_FEATURES',
> ['beta', 'theta'], 'yes', 'no', d)}"
> +
> +
> +def remove_prefixed_features(var, d):
> +    """Return the items to be removed from var with :remove.
> +
> +    This function is intended to be used in a :remove handler to remove
> +    items from a variable. It will interpret items prefixed with a '-' as
> +    items to be removed.
> +
> +    As an internal note, the 'remove_prefixed_features_internal' flag
> is used to
> +    avoid infinite recursion.
> +    """
> +    if d.getVarFlag(var, 'remove_prefixed_features_internal') == '1':
> +        return ''
> +
> +    from collections import Counter
> +
> +    d.setVarFlag(var, 'remove_prefixed_features_internal', '1')
> +    try:
> +        value = d.getVar(var)
> +        counter = Counter()
> +        for v in value.split():
> +            if v.startswith('-'):
> +                counter[v[1:]] -= 1
> +                counter[v] -= 1
> +            else:
> +                counter[v] += 1
> +        return ' '.join(v for v, c in counter.items() if c < 1)
> +    finally:
> +        d.delVarFlag(var, 'remove_prefixed_features_internal')
> +
> +def add_implied_features(var, implied_var, d):
> +    """Return the items to be appended due to the presence of other
> items in var.
> +
> +    This function is intended to be used in a :append handler to append
> +    items from a variable. It will rely on the mapping of implied features
> +    to append the corresponding items.
> +
> +    As an internal note, the 'add_implied_features_internal' flag is used to
> +    avoid infinite recursion.
> +    """
> +    if d.getVarFlag(var, 'add_implied_features_internal') == '1':
> +        return ''
> +
> +    def implied_features(feature, implied_mapping, d, seen=None):
> +        """Return the implied features for a given feature."""
> +        if seen is None:
> +            seen = set()
> +        if feature in seen:
> +            return ''
> +        seen.add(feature)
> +        implied = implied_mapping.get(feature, '').split()
> +        return ' '.join(implied + [implied_features(f,
> implied_mapping, d, seen) for f in implied])
> +
> +    d.setVarFlag(var, 'add_implied_features_internal', '1')
> +    try:
> +        value = d.getVar(var)
> +        implied_mapping = d.getVarFlags(implied_var)
> +        if implied_mapping is None:
> +            return ''
> +
> +        return ' '.join(implied_features(f, implied_mapping, d) for f
> in value.split())
> +    finally:
> +        d.delVarFlag(var, 'add_implied_features_internal')
> diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
> index cda98035..4dedb081 100644
> --- a/meta/conf/bitbake.conf
> +++ b/meta/conf/bitbake.conf
> @@ -172,6 +172,31 @@ BBINCLUDELOGS ??= "yes"
>  # Add event handlers for bitbake
>  INHERIT += "isar-events sstate"
> 
> +# Make features variables available
> +INHERIT += "features"
> +
> +BASE_REPO_FEATURES ??= ""
> +BASE_REPO_FEATURES[doc] = "Specifies the list of features for the
> base-apt repository."
> +BASE_REPO_FEATURES:remove =
> "${@remove_prefixed_features('BASE_REPO_FEATURES', d)}"
> +BASE_REPO_FEATURES:prepend =
> "${@add_implied_features('BASE_REPO_FEATURES',
> 'IMPLIED_BASE_REPO_FEATURES', d)} "
> +
> +MACHINE_FEATURES ??= ""
> +MACHINE_FEATURES[doc] = "Specifies the list of hardware features the
> MACHINE is capable of supporting."
> +MACHINE_FEATURES:remove = "${@remove_prefixed_features('MACHINE_FEATURES', d)}"
> +MACHINE_FEATURES:prepend =
> "${@add_implied_features('MACHINE_FEATURES',
> 'IMPLIED_MACHINE_FEATURES', d)} "
> +
> +DISTRO_FEATURES ??= ""
> +DISTRO_FEATURES[doc] = "The software support you want in your
> distribution for various features."
> +DISTRO_FEATURES:remove = "${@remove_prefixed_features('DISTRO_FEATURES', d)}"
> +DISTRO_FEATURES:prepend = "${@add_implied_features('DISTRO_FEATURES',
> 'IMPLIED_DISTRO_FEATURES', d)} "
> +
> +COMBINED_FEATURES = "${@oe.utils.set_intersect('DISTRO_FEATURES',
> 'MACHINE_FEATURES', d)}"
> +
> +ROOTFS_FEATURES ??= ""
> +ROOTFS_FEATURES[doc] = "The list of features to be included in a root
> filesystem. Typically, you configure this variable in an image recipe
> or class."
> +ROOTFS_FEATURES:remove = "${@remove_prefixed_features('ROOTFS_FEATURES', d)}"
> +ROOTFS_FEATURES:prepend = "${@add_implied_features('ROOTFS_FEATURES',
> 'IMPLIED_ROOTFS_FEATURES', d)} "
> +
>  # Buildstats requires IMAGE_ROOTFS to be always defined
>  IMAGE_ROOTFS ??= "${WORKDIR}/rootfs"
>  INHERIT += "${@'buildstats' if
> bb.utils.to_boolean(d.getVar('USE_BUILDSTATS')) else ''}"
> --
> 2.45.2
> 

Looks like it could be useful. I would just like to ensure that we are
not creating unneeded surprises for those coming from/going to OE/Yocto.
And bonus for a template-based setup.

Jan

-- 
Siemens AG, Technology
Linux Expert Center

-- 
You received this message because you are subscribed to the Google Groups "isar-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/isar-users/ad1008bd-9f8e-4626-8df9-ca4231b5287a%40siemens.com.

Re: [PATCH] RFC: features.bbclass,bitbake.conf: use features lists

['Larson, Chris' via isar-users]

On 11/14/2024 9:23 AM, 'Jan Kiszka' via isar-users wrote:
> On 11.11.24 17:51, Christopher Larson wrote:
>> Any thoughts on something like this? Too much?
>>
>> The intention is that downstreams would use standard bitbake functions
>> to check features, the same way they do in the Yocto Project, which
>> ensures that signatures and variable checksums never include the
>> entirety of the features variable, only the presence or absence of the
>> feature being checked.
>>
>> This also adds bits to allow one to disable a feature without having
>> to use the problematic :remove mechanism, which can't be undone by
>> downstream layers easily. In this case, MY_FEATURES += "feature-a" and
>> then MY_FEATURES += "-feature-a" would result in removing it. Order
>
> Is that "-feature" feature something upstream OE has as well? Or how do
> they handle that use case?

This isn't an upstream feature, though ideally it should be. Most just 
directly use :remove when necessary, but it does end up adding 
complications for downstreams unless they use intermediate variables, 
and even then you end up with workarounds like 
DISTRO_FEATURES_REMOVE:remove = "somefeature" to undo its removal, which 
is confusing :)

>> matters, so it can be re-added again later on without complication. It
>> also adds a minimal mechanism to let one feature pull in others
>> implicitly, which helps to avoid duplication when a feature is a
>> superset of another.
>
> Same question: Would that syntax or even the logic isar-specific or in
> line with OE?

This is also not present in OE at this time, though OE has similar 
functionality, implemented in similar ways, for example, the support for 
dependencies amongst image compression/conversion tools.

As these two are enhancements, not regressions, I don't think there 
would be any issue with someone migrating from OE to isar, only perhaps 
in the other direction. That said, I have no objection to pursuing their 
submission in OE-Core also.

> BTW, missing signed-off, but I that might be because of "RFC".

Indeed!

>> diff --git a/meta/classes/features.bbclass 
b/meta/classes/features.bbclass
>> new file mode 100644
>> index 00000000..32964c91
>> --- /dev/null
>> +++ b/meta/classes/features.bbclass
>> @@ -0,0 +1,91 @@
>
> Missing license/copyright header.

I'll fix that when I submit the non-RFC patch, thanks.

>> +# Functions to manipulate feature lists
>> +#
>> +# This class provides functions to manipulate feature lists. The 
functions
>> +# are intended to be used in :remove and :append handlers to remove 
or append
>> +# items to a variable. The remove will interpret items prefixed 
with a '-'
>> +# as items to be removed. The append function will rely on a mapping of
>> +# implied features to append the corresponding items. The intention 
is to
>> +# provide the ability for a feature to imply enabling of other 
features,
>> +# much like a dependency, but without the implication of order.
>> +#
>> +# Usage example:
>> +#
>> +#  IMAGE_FEATURES ??= ""
>> +#  IMAGE_FEATURES:remove = 
"${@remove_prefixed_features('IMAGE_FEATURES', d)}"
>> +#  IMAGE_FEATURES:prepend =
>> "${@add_implied_features('IMAGE_FEATURES', 'IMPLIED_IMAGE_FEATURES',
>> d)} "
>> +#  IMAGE_FEATURES_DISABLED = "${@' '.join(f for f in
>> remove_prefixed_features('IMAGE_FEATURES', d).split() if not
>> f.startswith('-'))}"
>
> What's the purpose of the last line? Or is it a stale example?

Remnant line, thanks.

> And then these three or four lines are kind of boilerplate for every
> feature group one would like to create? Would there be a way to template
> that with the feature variable name as parameter?

That's an excellent question, and one which I considered as well, but 
forgot to mention in the email and commit.

A big part of why I implemented these features as inline python in 
:remove and :append was to ensure that there are no issues with 
order-of-operations in the metadata, and to ensure that we don't break 
the bitbake checksumming for lists of items like features.

BitBake has special handling of the `contains` functions to ensure that 
a caller only depends on the presence or absence of the feature they 
care about, rather than depending on the entirety of the feature 
variable, as that would lead to very brittle checksums, and reduced 
shared state usage. If one was to provide wrapper functions to check for 
a feature and implement this logic there, it would bypass bitbake's 
handling in this regard.

The other option was to implement the removal and prepend at some 
specific point, for example, in an event handler (i.e. ConfigParsed, 
BuildStarted), or anonymous python. The problem with that has to due to 
with order of operations, as the behavior would be incorrect if one checks
for a feature before this code runs. Any code to 
automatically set the flags based on a list of supported features 
variables would run into the same issue. The bitbake file format is 
already rather prone to issues relating to when operations happen, so 
setting the flags immediately, and having them operate when the variable 
is checked, seemed like a reasonable approach.

There's an argument to be made that no one should be checking for 
features prior to ConfigParsed anyway, as we don't know if a later 
config file will add or remove a feature, so combining a contains() 
with, say, :=, would be error prone to begin with. This is a valid 
argument in favor of applying the templating at ConfigParsed-time, but I 
figured it was clearer to be explicit here, even if it led to a small 
amount of redundancy, rather than adding more order-dependent logic, 
particularly given the small number of supported features variables.

I'd certainly like to hear opinions on this, though. Thoughts?

> BTW, patch was mangled by your email client.

Ah, apparently using git imap-send to tweak the email before sending 
still ended up mangling it. Apologies. I'll use format-patch, tweak, and 
then send-email instead.

>> +#
>> +#  IMPLIED_IMAGE_FEATURES[alpha] = "beta"
>
> So, if I add "alpha" to IMAGE_FEATURES, I will also get "beta" as
> feature, right?

Exactly, yes.

> Looks like it could be useful. I would just like to ensure that we are
> not creating unneeded surprises for those coming from/going to OE Yocto.
> And bonus for a template-based setup.

Understood, thanks for the feedback. If anyone has any ideas on how to 
pull off the templating either without adding order-dependent 
operations, or feels strongly that it's worth doing so, I'd love to 
discuss that. Would you feel better if I pursue submission to oe-core first?
-- 
Christopher Larson
Siemens AG
www.siemens.com

-- 
You received this message because you are subscribed to the Google Groups "isar-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/isar-users/BN0PR07MB837594CAD54725861274EC729F202%40BN0PR07MB8375.namprd07.prod.outlook.com.