Patchwork base.bbclass: Add COMPATIBLE_DISTRO_FEATURES support

login
register
mail settings
Submitter Otavio Salvador
Date May 28, 2013, 3:04 p.m.
Message ID <1369753475-22573-1-git-send-email-otavio@ossystems.com.br>
Download mbox | patch
Permalink /patch/50629/
State New
Headers show

Comments

Otavio Salvador - May 28, 2013, 3:04 p.m.
This add support to list compatible distro feature for a recipe; this
is important for example when you have two different recipes which
should be choosen depending on the distro features set.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
---
 meta/classes/base.bbclass | 10 ++++++++++
 1 file changed, 10 insertions(+)
Phil Blundell - May 28, 2013, 3:12 p.m.
On Tue, 2013-05-28 at 12:04 -0300, Otavio Salvador wrote:
> This add support to list compatible distro feature for a recipe; this
> is important for example when you have two different recipes which
> should be choosen depending on the distro features set.

The terminology used here seems to be wrong/misleading/confusing:

> +        need_distro_features = d.getVar('COMPATIBLE_DISTRO_FEATURES', True)
> +        if need_distro_features:
> +            need_distro_features = need_distro_features.split()
> +            distro_features = (d.getVar('DISTRO_FEATURES', True) or "").split()
> +            for f in need_distro_features:
> +                if f in distro_features:
> +                    bb.note("here")
> +                    break
> +            else:
> +                raise bb.parse.SkipPackage("incompatible with distro features %s (not in DISTRO_FEATURES)" % need_distro_features)

Calling the variable "COMPATIBLE_DISTRO_FEATURES" implies that the
recipe won't work with any features not in the list.  But the actual
implementation shown above is that the recipe will be skipped unless all
of the features listed are present in DISTRO_FEATURES.  If those are the
intended semantics then it seems as though the variable ought to be
named REQUIRED_DISTRO_FEATURES or something.

Similarly, the error message "incompatible with distro features %s (not
in DISTRO_FEATURES)" is, at best, confusing.  This message seems to be
staying that the recipe won't work if the named features are set, but it
then goes on to say that they are not in fact set.  Plus, it lists the
entire contents of COMPATIBLE_DISTRO_FEATURES rather than just the items
that are causing a problem.

Also, you seem to have a spurious bb.note() call in there.

Also also, we've managed without this functionality in oe-core for some
time which makes me wonder how widely useful it would actually be.  If
it's only going to be used by a few recipes then it could go in a class,
or in in the recipes themselves, rather than adding parse time and
memory footprint to every recipe.

p.
Otavio Salvador - May 28, 2013, 3:18 p.m.
On Tue, May 28, 2013 at 12:12 PM, Phil Blundell <pb@pbcl.net> wrote:

> On Tue, 2013-05-28 at 12:04 -0300, Otavio Salvador wrote:
> > This add support to list compatible distro feature for a recipe; this
> > is important for example when you have two different recipes which
> > should be choosen depending on the distro features set.
>
> The terminology used here seems to be wrong/misleading/confusing:
>
> > +        need_distro_features = d.getVar('COMPATIBLE_DISTRO_FEATURES',
> True)
> > +        if need_distro_features:
> > +            need_distro_features = need_distro_features.split()
> > +            distro_features = (d.getVar('DISTRO_FEATURES', True) or
> "").split()
> > +            for f in need_distro_features:
> > +                if f in distro_features:
> > +                    bb.note("here")
> > +                    break
> > +            else:
> > +                raise bb.parse.SkipPackage("incompatible with distro
> features %s (not in DISTRO_FEATURES)" % need_distro_features)
>
> Calling the variable "COMPATIBLE_DISTRO_FEATURES" implies that the
> recipe won't work with any features not in the list.  But the actual
> implementation shown above is that the recipe will be skipped unless all
> of the features listed are present in DISTRO_FEATURES.  If those are the
> intended semantics then it seems as though the variable ought to be
> named REQUIRED_DISTRO_FEATURES or something.
>

Done.


> Similarly, the error message "incompatible with distro features %s (not
> in DISTRO_FEATURES)" is, at best, confusing.  This message seems to be
> staying that the recipe won't work if the named features are set, but it
> then goes on to say that they are not in fact set.  Plus, it lists the
> entire contents of COMPATIBLE_DISTRO_FEATURES rather than just the items
> that are causing a problem.
>

Done.


> Also, you seem to have a spurious bb.note() call in there.
>

Arrg, done.


> Also also, we've managed without this functionality in oe-core for some
> time which makes me wonder how widely useful it would actually be.  If
> it's only going to be used by a few recipes then it could go in a class,
> or in in the recipes themselves, rather than adding parse time and
> memory footprint to every recipe.
>

Right; let's first agree in the code so we can discuss where to put it:

+        need_distro_features = d.getVar('REQUIRED_DISTRO_FEATURES', True)
+        if need_distro_features:
+            need_distro_features = need_distro_features.split()
+            distro_features = (d.getVar('DISTRO_FEATURES', True) or
"").split()
+            for f in need_distro_features:
+                if f in distro_features:
+                    break
+            else:
+                raise bb.parse.SkipPackage("missing required distro
feature %s (not in DISTRO_FEATURES)" % need_distro_features)
Eric BENARD - May 28, 2013, 3:21 p.m.
Hi Otavio,

Le Tue, 28 May 2013 12:18:21 -0300,
Otavio Salvador <otavio@ossystems.com.br> a écrit :

> On Tue, May 28, 2013 at 12:12 PM, Phil Blundell <pb@pbcl.net> wrote:
> > Also also, we've managed without this functionality in oe-core for some
> > time which makes me wonder how widely useful it would actually be.  If
> > it's only going to be used by a few recipes then it could go in a class,
> > or in in the recipes themselves, rather than adding parse time and
> > memory footprint to every recipe.
> >
> 
> Right; let's first agree in the code so we can discuss where to put it:
> 
> +        need_distro_features = d.getVar('REQUIRED_DISTRO_FEATURES', True)
> +        if need_distro_features:
> +            need_distro_features = need_distro_features.split()
> +            distro_features = (d.getVar('DISTRO_FEATURES', True) or
> "").split()
> +            for f in need_distro_features:
> +                if f in distro_features:
> +                    break
> +            else:
> +                raise bb.parse.SkipPackage("missing required distro
> feature %s (not in DISTRO_FEATURES)" % need_distro_features)
> 
> 
If I follow you want that to avoid this line in a recipe :
COMPATIBLE_MACHINE = "{@base_contains('DISTRO_FEATURES', 'x11', (mx5),
'', d)}"
maybe there is a more clean way to exclude this recipe when x11
is not present without impacting the whole build system ?

Eric
Otavio Salvador - May 28, 2013, 3:26 p.m.
On Tue, May 28, 2013 at 12:21 PM, Eric Bénard <eric@eukrea.com> wrote:

> Hi Otavio,
>
> Le Tue, 28 May 2013 12:18:21 -0300,
> Otavio Salvador <otavio@ossystems.com.br> a écrit :
>
> > On Tue, May 28, 2013 at 12:12 PM, Phil Blundell <pb@pbcl.net> wrote:
> > > Also also, we've managed without this functionality in oe-core for some
> > > time which makes me wonder how widely useful it would actually be.  If
> > > it's only going to be used by a few recipes then it could go in a
> class,
> > > or in in the recipes themselves, rather than adding parse time and
> > > memory footprint to every recipe.
> > >
> >
> > Right; let's first agree in the code so we can discuss where to put it:
> >
> > +        need_distro_features = d.getVar('REQUIRED_DISTRO_FEATURES',
> True)
> > +        if need_distro_features:
> > +            need_distro_features = need_distro_features.split()
> > +            distro_features = (d.getVar('DISTRO_FEATURES', True) or
> > "").split()
> > +            for f in need_distro_features:
> > +                if f in distro_features:
> > +                    break
> > +            else:
> > +                raise bb.parse.SkipPackage("missing required distro
> > feature %s (not in DISTRO_FEATURES)" % need_distro_features)
> >
> >
> If I follow you want that to avoid this line in a recipe :
> COMPATIBLE_MACHINE = "{@base_contains('DISTRO_FEATURES', 'x11', (mx5),
> '', d)}"
> maybe there is a more clean way to exclude this recipe when x11
> is not present without impacting the whole build system ?
>

This is one place it'd be used. Wayland support is another.

It can be placed in a class, no problem ... I just want people to agree in
the code so I  can send a v2.
Phil Blundell - May 28, 2013, 3:31 p.m.
On Tue, 2013-05-28 at 12:18 -0300, Otavio Salvador wrote:

> +        need_distro_features = d.getVar('REQUIRED_DISTRO_FEATURES',
> True)
> +        if need_distro_features:
> +            need_distro_features = need_distro_features.split()
> +            distro_features = (d.getVar('DISTRO_FEATURES', True) or
> "").split()
> +            for f in need_distro_features:
> +                if f in distro_features:
> +                    break
> +            else:
> +                raise bb.parse.SkipPackage("missing required distro
> feature %s (not in DISTRO_FEATURES)" % need_distro_features)

This is still not quite right.  The error message says that the features
it mentions are "not in DISTRO_FEATURES", but it appears to still be
listing everything that's named in REQUIRED_DISTRO_FEATURES whether or
not it is actually in DISTRO_FEATURES.

Also you seem to be printing a Python list with %s, which will work but
the results aren't especially pretty.

Also also, on a stylistic point, the "for/if/break" construct is a bit
ugly.  You could perhaps consider something like:

need_distro_features = need_distro_features.split()
distro_features = (d.getVar('DISTRO_FEATURES', True) or "").split()
missing_distro_features = filter(lambda x: x not in distro_features, need_distro_features)
if missing_distro_features:
  raise bb.parse.SkipPackage("missing required distro features: %s" % " ".join(missing_distro_features))

p.
Otavio Salvador - May 28, 2013, 4:08 p.m.
On Tue, May 28, 2013 at 12:31 PM, Phil Blundell <pb@pbcl.net> wrote:

> On Tue, 2013-05-28 at 12:18 -0300, Otavio Salvador wrote:
>
> > +        need_distro_features = d.getVar('REQUIRED_DISTRO_FEATURES',
> > True)
> > +        if need_distro_features:
> > +            need_distro_features = need_distro_features.split()
> > +            distro_features = (d.getVar('DISTRO_FEATURES', True) or
> > "").split()
> > +            for f in need_distro_features:
> > +                if f in distro_features:
> > +                    break
> > +            else:
> > +                raise bb.parse.SkipPackage("missing required distro
> > feature %s (not in DISTRO_FEATURES)" % need_distro_features)
>
> This is still not quite right.  The error message says that the features
> it mentions are "not in DISTRO_FEATURES", but it appears to still be
> listing everything that's named in REQUIRED_DISTRO_FEATURES whether or
> not it is actually in DISTRO_FEATURES.
>
> Also you seem to be printing a Python list with %s, which will work but
> the results aren't especially pretty.
>
> Also also, on a stylistic point, the "for/if/break" construct is a bit
> ugly.  You could perhaps consider something like:
>
> need_distro_features = need_distro_features.split()
> distro_features = (d.getVar('DISTRO_FEATURES', True) or "").split()
> missing_distro_features = filter(lambda x: x not in distro_features,
> need_distro_features)
> if missing_distro_features:
>   raise bb.parse.SkipPackage("missing required distro features: %s" % "
> ".join(missing_distro_features))
>

I dislike the 'required' my idea here was each item of the 'compat distro
features' would be as an OR. Like

COMPATIBLE_DISTRO_FEATURE = "x11 wayland"

So *any* of those would make recipe to be parsed.
Phil Blundell - May 28, 2013, 4:28 p.m.
On Tue, 2013-05-28 at 13:08 -0300, Otavio Salvador wrote:

> I dislike the 'required' my idea here was each item of the 'compat
> distro features' would be as an OR. Like
> 
> 
> COMPATIBLE_DISTRO_FEATURE = "x11 wayland"

Oh, right, I see.  Sorry, I misunderstood the intent of your original
patch.

Well, then, how about making each item be a regex and having:

REQUIRED_DISTRO_FEATURES = "x11|wayland qt|gtk"

or some such?

All that said, before spending too much more time debating the minutiae
of this particular feature, I'd kind of like to get a better
understanding of what problem it's actually solving.  How many recipes
would actually need to use this mechanism and why?

p.
Otavio Salvador - May 28, 2013, 4:33 p.m.
On Tue, May 28, 2013 at 1:28 PM, Phil Blundell <pb@pbcl.net> wrote:

> On Tue, 2013-05-28 at 13:08 -0300, Otavio Salvador wrote:
>
> > I dislike the 'required' my idea here was each item of the 'compat
> > distro features' would be as an OR. Like
> >
> >
> > COMPATIBLE_DISTRO_FEATURE = "x11 wayland"
>
> Oh, right, I see.  Sorry, I misunderstood the intent of your original
> patch.
>
> Well, then, how about making each item be a regex and having:
>
> REQUIRED_DISTRO_FEATURES = "x11|wayland qt|gtk"
>
> or some such?
>

I did it initially using a regexp but I think it is too complexity for a
small thing. Besides the features has  a fixed value (different of machine
name and like) so a list would fit better.


> All that said, before spending too much more time debating the minutiae
> of this particular feature, I'd kind of like to get a better
> understanding of what problem it's actually solving.  How many recipes
> would actually need to use this mechanism and why?
>

In meta-fsl-arm, for now, it'll be 4 (gpu-viv-bin, gpu-viv-wl-bin,
amd-gpu-bin and amd-gpu-x11-bin) but I think other layers has same need.
Phil Blundell - May 28, 2013, 4:36 p.m.
On Tue, 2013-05-28 at 13:33 -0300, Otavio Salvador wrote:

> In meta-fsl-arm, for now, it'll be 4 (gpu-viv-bin, gpu-viv-wl-bin,
> amd-gpu-bin and amd-gpu-x11-bin) but I think other layers has same
> need.

Can you explain why those recipes need this feature?

p.
Otavio Salvador - May 28, 2013, 4:39 p.m.
On Tue, May 28, 2013 at 1:36 PM, Phil Blundell <pb@pbcl.net> wrote:

> On Tue, 2013-05-28 at 13:33 -0300, Otavio Salvador wrote:
>
> > In meta-fsl-arm, for now, it'll be 4 (gpu-viv-bin, gpu-viv-wl-bin,
> > amd-gpu-bin and amd-gpu-x11-bin) but I think other layers has same
> > need.
>
> Can you explain why those recipes need this feature?
>

gpu-viv and gpu-viv-wl provide support for Vivante GPU.

gpu-viv: x11
gpu-viv-wl: wayland

Patch

diff --git a/meta/classes/base.bbclass b/meta/classes/base.bbclass
index b1642a2..1c0b0d9 100644
--- a/meta/classes/base.bbclass
+++ b/meta/classes/base.bbclass
@@ -522,6 +522,16 @@  python () {
             else:
                 raise bb.parse.SkipPackage("incompatible with machine %s (not in COMPATIBLE_MACHINE)" % d.getVar('MACHINE', True))
 
+        need_distro_features = d.getVar('COMPATIBLE_DISTRO_FEATURES', True)
+        if need_distro_features:
+            need_distro_features = need_distro_features.split()
+            distro_features = (d.getVar('DISTRO_FEATURES', True) or "").split()
+            for f in need_distro_features:
+                if f in distro_features:
+                    bb.note("here")
+                    break
+            else:
+                raise bb.parse.SkipPackage("incompatible with distro features %s (not in DISTRO_FEATURES)" % need_distro_features)
 
         bad_licenses = (d.getVar('INCOMPATIBLE_LICENSE', True) or "").split()