| Message ID | 1369753475-22573-1-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | New |
| Headers | show |
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()
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.
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)
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
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.
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.
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.
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.
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.
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.
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
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(+)