[PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[martin.jansa]

From: Martin Jansa <Martin.Jansa@gmail.com>

* if there is multiple .bbappend files with FILESEXTRAPATHS_prepend := "/:"
  then the one parsed last is causing trailing ':' and that's causing empty element in
  path = extrapaths.split(:) + path
* it's hard to keep all .bbappends from foreign layers to follow this rule, so it's better
  to be able to handle trailing ':'

Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
---
 meta/classes/utils.bbclass |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/meta/classes/utils.bbclass b/meta/classes/utils.bbclass
index 56abdd8..3c2e342 100644
--- a/meta/classes/utils.bbclass
+++ b/meta/classes/utils.bbclass
@@ -338,8 +338,9 @@ def base_set_filespath(path, d):
 	# The ":" ensures we have an 'empty' override
 	overrides = (bb.data.getVar("OVERRIDES", d, 1) or "") + ":"
 	for p in path:
-		for o in overrides.split(":"):
-			filespath.append(os.path.join(p, o))
+		if p != "": 
+			for o in overrides.split(":"):
+				filespath.append(os.path.join(p, o))
 	return ":".join(filespath)
 
 def extend_variants(d, var, extend, delim=':'):
-- 
1.7.6

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Chris Larson]

On Fri, Aug 19, 2011 at 5:16 AM,  <martin.jansa@gmail.com> wrote:
> From: Martin Jansa <Martin.Jansa@gmail.com>
>
> * if there is multiple .bbappend files with FILESEXTRAPATHS_prepend := "/:"
>  then the one parsed last is causing trailing ':' and that's causing empty element in
>  path = extrapaths.split(:) + path
> * it's hard to keep all .bbappends from foreign layers to follow this rule, so it's better
>  to be able to handle trailing ':'
>
> Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
> ---
>  meta/classes/utils.bbclass |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/utils.bbclass b/meta/classes/utils.bbclass
> index 56abdd8..3c2e342 100644
> --- a/meta/classes/utils.bbclass
> +++ b/meta/classes/utils.bbclass
> @@ -338,8 +338,9 @@ def base_set_filespath(path, d):
>        # The ":" ensures we have an 'empty' override
>        overrides = (bb.data.getVar("OVERRIDES", d, 1) or "") + ":"
>        for p in path:
> -               for o in overrides.split(":"):
> -                       filespath.append(os.path.join(p, o))
> +               if p != "":
> +                       for o in overrides.split(":"):
> +                               filespath.append(os.path.join(p, o))

You shouldn't use 'p != "":'. Instead, use the fact that the empty
string is false in boolean context.  'if p:'.

Of course, you could also use filter. path = filter(None, path) --
filtering an iterable with a function of None results in it dropping
all false values. But then, there's a tendency nowadays to avoid
map/filter/etc, so that's probably not best :)
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Martin Jansa]

[-- Attachment #1: Type: text/plain, Size: 2022 bytes --]

On Fri, Aug 19, 2011 at 03:11:08PM -0700, Chris Larson wrote:
> On Fri, Aug 19, 2011 at 5:16 AM,  <martin.jansa@gmail.com> wrote:
> > From: Martin Jansa <Martin.Jansa@gmail.com>
> >
> > * if there is multiple .bbappend files with FILESEXTRAPATHS_prepend := "/:"
> >  then the one parsed last is causing trailing ':' and that's causing empty element in
> >  path = extrapaths.split(:) + path
> > * it's hard to keep all .bbappends from foreign layers to follow this rule, so it's better
> >  to be able to handle trailing ':'
> >
> > Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
> > ---
> >  meta/classes/utils.bbclass |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/meta/classes/utils.bbclass b/meta/classes/utils.bbclass
> > index 56abdd8..3c2e342 100644
> > --- a/meta/classes/utils.bbclass
> > +++ b/meta/classes/utils.bbclass
> > @@ -338,8 +338,9 @@ def base_set_filespath(path, d):
> >        # The ":" ensures we have an 'empty' override
> >        overrides = (bb.data.getVar("OVERRIDES", d, 1) or "") + ":"
> >        for p in path:
> > -               for o in overrides.split(":"):
> > -                       filespath.append(os.path.join(p, o))
> > +               if p != "":
> > +                       for o in overrides.split(":"):
> > +                               filespath.append(os.path.join(p, o))
> 
> You shouldn't use 'p != "":'. Instead, use the fact that the empty
> string is false in boolean context.  'if p:'.
> 
> Of course, you could also use filter. path = filter(None, path) --
> filtering an iterable with a function of None results in it dropping
> all false values. But then, there's a tendency nowadays to avoid
> map/filter/etc, so that's probably not best :)

does it apply to all != "" in utils.bbclass? because I've used it just
because it's at least 3 times in this file already.. so to be consistent
with the rest..

Regards,
-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 205 bytes --]

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Paul Eggleton]

On Friday 19 August 2011 23:16:53 Martin Jansa wrote:
> On Fri, Aug 19, 2011 at 03:11:08PM -0700, Chris Larson wrote:
> > On Fri, Aug 19, 2011 at 5:16 AM,  <martin.jansa@gmail.com> wrote:
> > > From: Martin Jansa <Martin.Jansa@gmail.com>
> > > 
> > > * if there is multiple .bbappend files with FILESEXTRAPATHS_prepend :=
> > > "/:" then the one parsed last is causing trailing ':' and that's
> > > causing empty element in path = extrapaths.split(:) + path
> > > * it's hard to keep all .bbappends from foreign layers to follow this
> > > rule, so it's better to be able to handle trailing ':'
> > > 
> > > Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
> > > ---
> > >  meta/classes/utils.bbclass |    5 +++--
> > >  1 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/meta/classes/utils.bbclass b/meta/classes/utils.bbclass
> > > index 56abdd8..3c2e342 100644
> > > --- a/meta/classes/utils.bbclass
> > > +++ b/meta/classes/utils.bbclass
> > > @@ -338,8 +338,9 @@ def base_set_filespath(path, d):
> > >        # The ":" ensures we have an 'empty' override
> > >        overrides = (bb.data.getVar("OVERRIDES", d, 1) or "") + ":"
> > >        for p in path:
> > > -               for o in overrides.split(":"):
> > > -                       filespath.append(os.path.join(p, o))
> > > +               if p != "":
> > > +                       for o in overrides.split(":"):
> > > +                               filespath.append(os.path.join(p, o))
> > 
> > You shouldn't use 'p != "":'. Instead, use the fact that the empty
> > string is false in boolean context.  'if p:'.
> > 
> > Of course, you could also use filter. path = filter(None, path) --
> > filtering an iterable with a function of None results in it dropping
> > all false values. But then, there's a tendency nowadays to avoid
> > map/filter/etc, so that's probably not best :)
> 
> does it apply to all != "" in utils.bbclass? because I've used it just
> because it's at least 3 times in this file already.. so to be consistent
> with the rest..

FWIW I agree with Martin; I wouldn't hold back the patch just for this reason 
- it's not incorrect code, it's consistent with the rest of the file, and we 
can easily clean these up later.

Acked-by: Paul Eggleton <paul.eggleton@linux.intel.com>

- Paul
-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Chris Larson]

On Mon, Aug 22, 2011 at 6:01 AM, Paul Eggleton
<paul.eggleton@linux.intel.com> wrote:
> FWIW I agree with Martin; I wouldn't hold back the patch just for this reason
> - it's not incorrect code, it's consistent with the rest of the file, and we
> can easily clean these up later.

I strongly disagree with this. The fact is, we almost never go back
and "clean these up later", so crap accrues.

-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Paul Eggleton]

On Thursday 25 August 2011 00:16:40 Chris Larson wrote:
> I strongly disagree with this. The fact is, we almost never go back
> and "clean these up later", so crap accrues.

We're talking about the difference between:

if a = "":

and 

if a:

This is not crap, it's a triviality (for the case where the functional 
difference is not important anyway). If you care then by all means submit a 
patch to change it.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Chris Larson]

On Wed, Aug 24, 2011 at 5:16 PM, Paul Eggleton
<paul.eggleton@linux.intel.com> wrote:
> On Thursday 25 August 2011 00:16:40 Chris Larson wrote:
>> I strongly disagree with this. The fact is, we almost never go back
>> and "clean these up later", so crap accrues.
>
> We're talking about the difference between:
>
> if a = "":
>
> and
>
> if a:
>
> This is not crap, it's a triviality (for the case where the functional
> difference is not important anyway). If you care then by all means submit a
> patch to change it.

Yes, this example is trivial, but its just one of many instances of
this sort of code going in. I'd rather get the code clean to begin
with, rather than letting crap pile up and never do anything about it.
But I realize I care more about code quality than most of the people
in this project.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Richard Purdie]

On Wed, 2011-08-24 at 17:20 -0700, Chris Larson wrote:
> On Wed, Aug 24, 2011 at 5:16 PM, Paul Eggleton
> <paul.eggleton@linux.intel.com> wrote:
> > On Thursday 25 August 2011 00:16:40 Chris Larson wrote:
> >> I strongly disagree with this. The fact is, we almost never go back
> >> and "clean these up later", so crap accrues.
> >
> > We're talking about the difference between:
> >
> > if a = "":
> >
> > and
> >
> > if a:
> >
> > This is not crap, it's a triviality (for the case where the functional
> > difference is not important anyway). If you care then by all means submit a
> > patch to change it.
> 
> Yes, this example is trivial, but its just one of many instances of
> this sort of code going in. I'd rather get the code clean to begin
> with, rather than letting crap pile up and never do anything about it.
> But I realize I care more about code quality than most of the people
> in this project.

I'm not sure that is true, I think many people care about the code
quality of OE. Quality means different things to different people. We
also all have different priorities. It would be nice in many ways if our
biggest worry was these sorts of issues but it isn't.

Also, we have a lot of new people looking at the project and starting to
contribute. People are learning a lot and becoming very skilled
developers, its exiting to watch some of their skill sets grow. Taking
an all or nothing approach to accepting patches tends to be
counterproductive. Educating people over time tends to work well.

I think these things will get addressed and get better in general over
time (and by many measures quality is improving already).

Cheers,

Richard

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Chris Larson]

On Wed, Aug 24, 2011 at 6:24 PM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> On Wed, 2011-08-24 at 17:20 -0700, Chris Larson wrote:
>> On Wed, Aug 24, 2011 at 5:16 PM, Paul Eggleton
>> <paul.eggleton@linux.intel.com> wrote:
>> > On Thursday 25 August 2011 00:16:40 Chris Larson wrote:
>> >> I strongly disagree with this. The fact is, we almost never go back
>> >> and "clean these up later", so crap accrues.
>> >
>> > We're talking about the difference between:
>> >
>> > if a = "":
>> >
>> > and
>> >
>> > if a:
>> >
>> > This is not crap, it's a triviality (for the case where the functional
>> > difference is not important anyway). If you care then by all means submit a
>> > patch to change it.
>>
>> Yes, this example is trivial, but its just one of many instances of
>> this sort of code going in. I'd rather get the code clean to begin
>> with, rather than letting crap pile up and never do anything about it.
>> But I realize I care more about code quality than most of the people
>> in this project.
>
> I'm not sure that is true, I think many people care about the code
> quality of OE. Quality means different things to different people. We
> also all have different priorities. It would be nice in many ways if our
> biggest worry was these sorts of issues but it isn't.

Biggest worry or not, I think choosing to push off things which are
trivial to improve now is a flawed approach in general, and is likely
to degrade quality over time. It's a question of philosophy and
approach. Choosing to do it as well as we can the first time versus
postponing it to a future time which may well never arrive. At this
point I can't say with any degree of confidence, personally, that our
overall quality is rising rather than lowering.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Phil Blundell]

On Fri, 2011-08-19 at 15:11 -0700, Chris Larson wrote:
> But then, there's a tendency nowadays to avoid map/filter/etc, 
> so that's probably not best :)

Oh, is there?  What's the objection to those constructs, out of
interest?  I am quite fond of them myself.

p.

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Chris Larson]

On Thu, Aug 25, 2011 at 2:18 AM, Phil Blundell <philb@gnu.org> wrote:
> On Fri, 2011-08-19 at 15:11 -0700, Chris Larson wrote:
>> But then, there's a tendency nowadays to avoid map/filter/etc,
>> so that's probably not best :)
>
> Oh, is there?  What's the objection to those constructs, out of
> interest?  I am quite fond of them myself.

Good question, I'm not sure what the reasoning is. I've read multiple
documents around discussing idiomatic python that seem to feel that
they should generally be dropped in favor of comprehensions and
generator expressions. But, using that for something like this just
looks rather silly in my opinion: [foo for foo in bar if foo].
*shrug*.

Personally, I'm quite a fan of the functional programming methodology,
but I do think using the functional style too much in python code
tends to result in code which is extremely dense, and therefore harder
to read, just due to how the language is, whether you compose
generator expressions and comprehensions or you use map/filter/etc.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

Re: [PATCH] utils.bbclass: skip empty paths when handling FILESEXTRAPATHS

[Saul Wold]

On 08/19/2011 05:16 AM, martin.jansa@gmail.com wrote:
> From: Martin Jansa<Martin.Jansa@gmail.com>
>
> * if there is multiple .bbappend files with FILESEXTRAPATHS_prepend := "/:"
>    then the one parsed last is causing trailing ':' and that's causing empty element in
>    path = extrapaths.split(:) + path
> * it's hard to keep all .bbappends from foreign layers to follow this rule, so it's better
>    to be able to handle trailing ':'
>
> Signed-off-by: Martin Jansa<Martin.Jansa@gmail.com>
> ---
>   meta/classes/utils.bbclass |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/utils.bbclass b/meta/classes/utils.bbclass
> index 56abdd8..3c2e342 100644
> --- a/meta/classes/utils.bbclass
> +++ b/meta/classes/utils.bbclass
> @@ -338,8 +338,9 @@ def base_set_filespath(path, d):
>   	# The ":" ensures we have an 'empty' override
>   	overrides = (bb.data.getVar("OVERRIDES", d, 1) or "") + ":"
>   	for p in path:
> -		for o in overrides.split(":"):
> -			filespath.append(os.path.join(p, o))
> +		if p != "":
> +			for o in overrides.split(":"):
> +				filespath.append(os.path.join(p, o))
>   	return ":".join(filespath)
>
>   def extend_variants(d, var, extend, delim=':'):

Merged into OE-Core

Thanks
	Sau!