Patchwork [2/2] base: make feature backfilling happen earlier

login
register
mail settings
Submitter Ross Burton
Date Jan. 24, 2013, 5:52 p.m.
Message ID <6d174abd95de70a59f54ca28c965ca72933a3ce7.1359049755.git.ross.burton@intel.com>
Download mbox | patch
Permalink /patch/43299/
State Accepted
Commit 22429cdf79ed952072707a929643c7386fa7e056
Headers show

Comments

Ross Burton - Jan. 24, 2013, 5:52 p.m.
From: Richard Purdie <richard.purdie@linuxfoundation.org>

Backfilling DISTRO_FEATURES and MACHINE_FEATURES with _append statements happens
too late to use those variables with conditional inherits, like this:

inherit ${@base_contains('DISTRO_FEATURES','sysvinit','update-rc.d_real','',d)}

Instead, do the backfilling at ConfigParse time so that it happens earlier in
the parse, which results in that inherit behaving as expected when sysvinit was
backfilled.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Signed-off-by: Ross Burton <ross.burton@intel.com>
---
 meta/classes/base.bbclass |    2 ++
 meta/conf/bitbake.conf    |    3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)
Enrico Scholz - Jan. 25, 2013, 6:06 p.m.
Ross Burton <ross.burton-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
writes:

> Backfilling DISTRO_FEATURES and MACHINE_FEATURES with _append statements
> happens too late to use those variables with conditional inherits, like
> this:

This causes regressions for setups where DISTRO_FEATURES_BACKFILL uses
the override mechanism. E.g. I have

| DISTRO_FEATURES_BACKFILL_mydist = "ld-is-gold ${${PROJECT_FEATURES}"
| DISTRO_FEATURES_INITMAN_mydist = "systemd"

Before this commit, DISTRO_FEATURES contained 'ld-is-gold' and the
project features.  Now, project features + ld-is-gold vanished and
pulseaudio is in again.



Enrico
Richard Purdie - Jan. 28, 2013, 12:54 p.m.
On Fri, 2013-01-25 at 19:06 +0100, Enrico Scholz wrote:
> Ross Burton <ross.burton-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> writes:
> 
> > Backfilling DISTRO_FEATURES and MACHINE_FEATURES with _append statements
> > happens too late to use those variables with conditional inherits, like
> > this:
> 
> This causes regressions for setups where DISTRO_FEATURES_BACKFILL uses
> the override mechanism. E.g. I have
> 
> | DISTRO_FEATURES_BACKFILL_mydist = "ld-is-gold ${${PROJECT_FEATURES}"
> | DISTRO_FEATURES_INITMAN_mydist = "systemd"
> 
> Before this commit, DISTRO_FEATURES contained 'ld-is-gold' and the
> project features.  Now, project features + ld-is-gold vanished and
> pulseaudio is in again.

I don't think its possible to make everything work :(

The question is therefore how do we want to proceed? I'll take further
proposed patches but I don't know what the best thing to do here is...

Cheers,

Richard
Enrico Scholz - Jan. 28, 2013, 1:55 p.m.
Richard Purdie <richard.purdie@linuxfoundation.org> writes:

>> > Backfilling DISTRO_FEATURES and MACHINE_FEATURES with _append statements
>> > happens too late to use those variables with conditional inherits, like
>> > this:
>> 
>> This causes regressions for setups where DISTRO_FEATURES_BACKFILL uses
>> the override mechanism. E.g. I have
>> 
>> | DISTRO_FEATURES_BACKFILL_mydist = "ld-is-gold ${${PROJECT_FEATURES}"
>> | DISTRO_FEATURES_INITMAN_mydist = "systemd"
>> 
>> Before this commit, DISTRO_FEATURES contained 'ld-is-gold' and the
>> project features.  Now, project features + ld-is-gold vanished and
>> pulseaudio is in again.
>
> I don't think its possible to make everything work :(
>
> The question is therefore how do we want to proceed? I'll take further
> proposed patches but I don't know what the best thing to do here is...

I made my setup (--> overriding DISTRO_FEATURES_BACKFILL by distro
configuration) working again by assigning it weakly in bitbake.conf:

-DISTRO_FEATURES_BACKFILL = "pulseaudio ${DISTRO_FEATURES_INITMAN}"
+DISTRO_FEATURES_BACKFILL ?= "pulseaudio ${DISTRO_FEATURES_INITMAN}"

ditto for MACHINE_FEATURES_BACKFILL.



Enrico
Paul Eggleton - Jan. 28, 2013, 2:19 p.m.
On Monday 28 January 2013 14:55:05 Enrico Scholz wrote:
> Richard Purdie <richard.purdie@linuxfoundation.org> writes:
> >> > Backfilling DISTRO_FEATURES and MACHINE_FEATURES with _append
> >> > statements
> >> > happens too late to use those variables with conditional inherits, like
> >> 
> >> > this:
> >> This causes regressions for setups where DISTRO_FEATURES_BACKFILL uses
> >> the override mechanism. E.g. I have
> >> 
> >> | DISTRO_FEATURES_BACKFILL_mydist = "ld-is-gold ${${PROJECT_FEATURES}"
> >> | DISTRO_FEATURES_INITMAN_mydist = "systemd"
> >> 
> >> Before this commit, DISTRO_FEATURES contained 'ld-is-gold' and the
> >> project features.  Now, project features + ld-is-gold vanished and
> >> pulseaudio is in again.
> > 
> > I don't think its possible to make everything work :(
> > 
> > The question is therefore how do we want to proceed? I'll take further
> > proposed patches but I don't know what the best thing to do here is...
> 
> I made my setup (--> overriding DISTRO_FEATURES_BACKFILL by distro
> configuration) working again by assigning it weakly in bitbake.conf:
> 
> -DISTRO_FEATURES_BACKFILL = "pulseaudio ${DISTRO_FEATURES_INITMAN}"
> +DISTRO_FEATURES_BACKFILL ?= "pulseaudio ${DISTRO_FEATURES_INITMAN}"
> 
> ditto for MACHINE_FEATURES_BACKFILL.

Why are you assigning *_FEATURES_BACKFILL at all?

Cheers,
Paul
Enrico Scholz - Jan. 28, 2013, 2:52 p.m.
Paul Eggleton <paul.eggleton@linux.intel.com> writes:

> Why are you assigning *_FEATURES_BACKFILL at all?

There are some implicit relations between features (e.g. when selecting
'usbhost' I *usually* want 'vfat' or 'ext2' support too). So I have

  DISTRO_FEATURES_BACKFILL = "\
    largefile nfsroot modules ld-is-gold ${PROJECT_FEATURES} \
    ${DISTRO_FEATURES_INITMAN} \
    ${@base_contains('PROJECT_FEATURES', 'alsa', 'sound', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'directfb', 'screen', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'fb', 'screen', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'mmc', 'ext2 vfat', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'touchscreen', 'screen', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'ubifs', 'mtd', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'jffs2', 'mtd', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'usbclient', 'usb usbgadget', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'usbgadget', 'usb usbclient', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'usbhost', 'ext2 vfat', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'usbhost', 'usb', '', d)} \
    ${@base_contains('PROJECT_FEATURES', 'x11', 'screen', '', d)} \
  "
  
  DISTRO_FEATURES_BACKFILL_CONSIDERED = "${NO_PROJECT_FEATURES}"

in my distro configuration (I knew; some are machine but no distro
features but this does not matter for me) and want to allow projects to
add new features and to remove default ones.

The default 'pulseaudio' in DISTRO_FEATURES_BACKFILL does not sound sane
to me either...


Enrico
Phil Blundell - Jan. 28, 2013, 3 p.m.
On Mon, 2013-01-28 at 15:52 +0100, Enrico Scholz wrote:
> There are some implicit relations between features (e.g. when selecting
> 'usbhost' I *usually* want 'vfat' or 'ext2' support too). So I have
> 
>   DISTRO_FEATURES_BACKFILL = "\
>     largefile nfsroot modules ld-is-gold ${PROJECT_FEATURES} \
>     ${DISTRO_FEATURES_INITMAN} \
>     ${@base_contains('PROJECT_FEATURES', 'alsa', 'sound', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'directfb', 'screen', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'fb', 'screen', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'mmc', 'ext2 vfat', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'touchscreen', 'screen', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'ubifs', 'mtd', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'jffs2', 'mtd', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'usbclient', 'usb usbgadget', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'usbgadget', 'usb usbclient', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'usbhost', 'ext2 vfat', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'usbhost', 'usb', '', d)} \
>     ${@base_contains('PROJECT_FEATURES', 'x11', 'screen', '', d)} \
>   "
>   
>   DISTRO_FEATURES_BACKFILL_CONSIDERED = "${NO_PROJECT_FEATURES}"
> 
> in my distro configuration (I knew; some are machine but no distro
> features but this does not matter for me) and want to allow projects to
> add new features and to remove default ones.

This does seem rather like an abuse of DISTRO_FEATURES_BACKFILL.  Can
you explain why you are doing it this way rather than just setting
DISTRO_FEATURES directly to what you wanted?

p.
Enrico Scholz - Jan. 28, 2013, 3:44 p.m.
Phil Blundell <pb@pbcl.net> writes:

> This does seem rather like an abuse of DISTRO_FEATURES_BACKFILL.  Can
> you explain why you are doing it this way rather than just setting
> DISTRO_FEATURES directly to what you wanted?

I need a way to:

1. set some defaults on distribution base and avoid nasty details like
   mandatory ${DISTRO_FEATURES_LIBC} flags in the project configuration

2. allow to override these defaults on a per-project base

3. add features support by recipes/classes of my distribution

afaik, DISTRO_FEATURES_BACKFILL + _CONSIDERED exist to allow the first
two point without an '-=' operator which lacks in bitbake.

Of course, I could reinvent the wheel and write my own _CONSIDERED
mechanism.  But until now (resp. without the '=' => '?=' change), it
worked fine with DISTRO_FEATURES_BACKFILL.


Enrico
Paul Eggleton - Jan. 28, 2013, 3:58 p.m.
On Monday 28 January 2013 16:44:15 Enrico Scholz wrote:
> Phil Blundell <pb@pbcl.net> writes:
> > This does seem rather like an abuse of DISTRO_FEATURES_BACKFILL.  Can
> > you explain why you are doing it this way rather than just setting
> > DISTRO_FEATURES directly to what you wanted?
> 
> I need a way to:
> 
> 1. set some defaults on distribution base and avoid nasty details like
>    mandatory ${DISTRO_FEATURES_LIBC} flags in the project configuration
> 
> 2. allow to override these defaults on a per-project base
> 
> 3. add features support by recipes/classes of my distribution
> 
> afaik, DISTRO_FEATURES_BACKFILL + _CONSIDERED exist to allow the first
> two point without an '-=' operator which lacks in bitbake.

No they don't. They exist to allow adding new features that should be enabled 
for all existing distro configs without each of those having to be changed, and 
provide a means for distros to opt out of that enabling if they wish.

> Of course, I could reinvent the wheel and write my own _CONSIDERED
> mechanism.  But until now (resp. without the '=' => '?=' change), it
> worked fine with DISTRO_FEATURES_BACKFILL.

We don't support using the backfill mechanism in this manner; so if it breaks 
you get to keep both pieces.

It's still not clear to me why you could not just set DISTRO_FEATURES 
directly. If you have values you're likely to want to remove, you can put them 
in separate variables that you can clear out later.

Cheers,
Paul
Phil Blundell - Jan. 28, 2013, 4:15 p.m.
On Mon, 2013-01-28 at 16:44 +0100, Enrico Scholz wrote:
> afaik, DISTRO_FEATURES_BACKFILL + _CONSIDERED exist to allow the first
> two point without an '-=' operator which lacks in bitbake.

That wasn't really the intention.  The original purpose of
DISTRO_FEATURES_BACKFILL was to allow behaviour that currently defaults
to "on" to be brought under the control of a DISTRO_FEATURE without
breaking existing distros that were relying on it.

The idea was that DISTROs would set DISTRO_FEATURES_BACKFILL_CONSIDERED
to list the features that they are aware of but don't want (and would
never alter the value of DISTRO_FEATURES_BACKFILL).  Any feature that
ends up in the latter but not in the former is considered to post-date
the DISTRO and is, consequently, spliced into DISTRO_FEATURES by the
backwards-compatibility code.

Admittedly I'm not sure that this was ever documented anywhere other
than the mailing list archives so I guess it's understandable that you
(and quite likely others) are going a bit off-piste with it.  But the
primary use-case is what I described above, and this is the one that
needs to take precedence if there is a conflict between differing
requirements.

Incidentally, regarding your other comment about pulseaudio not
belonging in DISTRO_FEATURES_BACKFILL, it transpires that pulseaudio was
in fact the very feature that spurred the creation of this mechanism and
hence the first to be backfilled.  See:

http://thread.gmane.org/gmane.comp.handhelds.openembedded.core/10941/focus=13033

and the following thread.

p.
Enrico Scholz - Jan. 28, 2013, 4:23 p.m.
Paul Eggleton <paul.eggleton@linux.intel.com> writes:

>> 1. set some defaults on distribution base ...
>> 2. allow to override these defaults on a per-project base
>> ...
>> afaik, DISTRO_FEATURES_BACKFILL + _CONSIDERED exist to allow the first
>> two point without an '-=' operator which lacks in bitbake.
>
> No they don't. They exist to allow adding new features that should be enabled 
> for all existing distro configs without each of those having to be changed, and 
> provide a means for distros to opt out of that enabling if they wish.

How is this different from my requirements?  Ok; atm,
DISTRO_FEATURES_BACKFILL is pretty useless because it contains only
'pulseaudio' which is probably unwanted on >90% of all embedded devices
and it is impossible to extend DISTRO_FEATURES_BACKFILL.

But this variable exists to allow removal of features which are enabled
by default.


>> Of course, I could reinvent the wheel and write my own _CONSIDERED
>> mechanism.  But until now (resp. without the '=' => '?=' change), it
>> worked fine with DISTRO_FEATURES_BACKFILL.
> ...
> It's still not clear to me why you could not just set DISTRO_FEATURES
> directly. If you have values you're likely to want to remove, you can
> put them in separate variables that you can clear out later.

It is very difficultly to do within distro.conf. E.g. having

| DISTRO_FEATURES = "${@subtract_set('DISTRO_FEATURES', 'UNWANTED_FEATURES')}'

will probably result into some error regarding recursive expansion.

Using ':=' will not work here because required operations are not
available yet.


At the end it will reinvent the DISTRO_FEATURES_BACKFILL_CONSIDERED
wheel by moving it into my-distro.bbclass.



Enrico
Paul Eggleton - Jan. 28, 2013, 4:34 p.m.
On Monday 28 January 2013 17:23:40 Enrico Scholz wrote:
> Paul Eggleton <paul.eggleton@linux.intel.com> writes:
> >> 1. set some defaults on distribution base ...
> >> 2. allow to override these defaults on a per-project base
> >> ...
> >> afaik, DISTRO_FEATURES_BACKFILL + _CONSIDERED exist to allow the first
> >> two point without an '-=' operator which lacks in bitbake.
> > 
> > No they don't. They exist to allow adding new features that should be
> > enabled for all existing distro configs without each of those having to
> > be changed, and provide a means for distros to opt out of that enabling
> > if they wish.
> How is this different from my requirements?  Ok; atm,
> DISTRO_FEATURES_BACKFILL is pretty useless because it contains only
> 'pulseaudio' which is probably unwanted on >90% of all embedded devices

pulseaudio was enabled already in the recipes that care about it; that was the 
point of adding it in the first place so it could be turned off.

> and it is impossible to extend DISTRO_FEATURES_BACKFILL.

It's not intended to be extended; it's for use in OE-Core alone.

> > It's still not clear to me why you could not just set DISTRO_FEATURES
> > directly. If you have values you're likely to want to remove, you can
> > put them in separate variables that you can clear out later.
> 
> It is very difficultly to do within distro.conf. E.g. having
> 
> | DISTRO_FEATURES = "${@subtract_set('DISTRO_FEATURES',
> | 'UNWANTED_FEATURES')}'
> will probably result into some error regarding recursive expansion.

What I meant was something like:

DISTRO_FEATURES = "${DISTRO_FEATURES_BASIC} ${DISTRO_FEATURES_A}  
${DISTRO_FEATURES_B}"

And then in your specific configs:
DISTRO_FEATURES_A = ""

Cheers,
Paul
Enrico Scholz - Jan. 29, 2013, 11:07 a.m.
Paul Eggleton <paul.eggleton@linux.intel.com> writes:

>> >> 1. set some defaults on distribution base ...
>> >> 2. allow to override these defaults on a per-project base

fwiw, I am using now

_EXT_PROJECT_FEATURES = "\
  largefile nfsroot modules ld-is-gold ${PROJECT_FEATURES} \
  ${DISTRO_FEATURES_INITMAN} ${DISTRO_FEATURES_LIBC} \
  ${@base_contains('PROJECT_FEATURES', 'alsa', 'sound', '', d)} \
  ${@base_contains('PROJECT_FEATURES', 'directfb', 'screen', '', d)} \
  ...

DISTRO_FEATURES = "${@' '.join(sorted(list( \
  set(d.getVar('_EXT_PROJECT_FEATURES', True).split()) - \
  set(d.getVar('NO_PROJECT_FEATURES', True).split()))))}"

DISTRO_FEATURES_BACKFILL_CONSIDERED = "${DISTRO_FEATURES_BACKFILL}"



Enrico

Patch

diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
index b427a5e..dafded3 100644
--- a/meta/classes/base.bbclass
+++ b/meta/classes/base.bbclass
@@ -314,6 +314,8 @@  python base_eventhandler() {
         generate_git_config(e)
         pkgarch_mapping(e.data)
         preferred_ml_updates(e.data)
+        e.data.appendVar('DISTRO_FEATURES', oe.utils.features_backfill("DISTRO_FEATURES", e.data))
+        e.data.appendVar('MACHINE_FEATURES', oe.utils.features_backfill("MACHINE_FEATURES", e.data))
 
     if isinstance(e, bb.event.BuildStarted):
         statuslines = []
diff --git a/meta/conf/bitbake.conf b/meta/conf/bitbake.conf
index 607fb70..13a3ff9 100644
--- a/meta/conf/bitbake.conf
+++ b/meta/conf/bitbake.conf
@@ -739,10 +739,7 @@  MACHINE_ESSENTIAL_EXTRA_RRECOMMENDS ?= ""
 IMAGE_FEATURES += "${EXTRA_IMAGE_FEATURES}"
 
 DISTRO_FEATURES_BACKFILL = "pulseaudio ${DISTRO_FEATURES_INITMAN}"
-DISTRO_FEATURES_append = "${@oe.utils.features_backfill("DISTRO_FEATURES",d)}"
-
 MACHINE_FEATURES_BACKFILL = "rtc"
-MACHINE_FEATURES_append = "${@oe.utils.features_backfill("MACHINE_FEATURES",d)}"
 
 COMBINED_FEATURES = "\
     ${@base_both_contain("DISTRO_FEATURES", "MACHINE_FEATURES", "alsa", d)} \