[PATCH] data_smart: Drop default expand=False to getVar [API change]

[PATCH] data_smart: Drop default expand=False to getVar [API change]

[Richard Purdie]

At some point in the future, getVar should expand by default. To get
there from the current position, we need a period of time where the
expand parameter is mandatory.

This patch starts that process. Clear errors will result from any code
which doesn't provide this. Layers can be fixed with an expression
like:

sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep -ril getVar *`

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

diff --git a/bitbake/lib/bb/data_smart.py b/bitbake/lib/bb/data_smart.py
index 8c3e9ef..79bec1b 100644
--- a/bitbake/lib/bb/data_smart.py
+++ b/bitbake/lib/bb/data_smart.py
@@ -566,7 +566,7 @@ class DataSmart(MutableMapping):
                 if len(shortvar) == 0:
                     override = None
 
-    def getVar(self, var, expand=False, noweakdefault=False, parsing=False):
+    def getVar(self, var, expand, noweakdefault=False, parsing=False):
         return self.getVarFlag(var, "_content", expand, noweakdefault, parsing)
 
     def renameVar(self, key, newkey, **loginfo):

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Otavio Salvador]

On Tue, Feb 2, 2016 at 9:55 PM, Richard Purdie
<richard.purdie@linuxfoundation.org> wrote:
> At some point in the future, getVar should expand by default. To get
> there from the current position, we need a period of time where the
> expand parameter is mandatory.
>
> This patch starts that process. Clear errors will result from any code
> which doesn't provide this. Layers can be fixed with an expression
> like:
>
> sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep -ril getVar *`
>
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>

I am in favor of making it mandatory but I fail to see what we gain in
making it expand by default in future.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Richard Purdie]

On Wed, 2016-02-03 at 08:11 -0200, Otavio Salvador wrote:
> On Tue, Feb 2, 2016 at 9:55 PM, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > At some point in the future, getVar should expand by default. To
> > get
> > there from the current position, we need a period of time where the
> > expand parameter is mandatory.
> > 
> > This patch starts that process. Clear errors will result from any
> > code
> > which doesn't provide this. Layers can be fixed with an expression
> > like:
> > 
> > sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep 
> > -ril getVar *`
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> I am in favor of making it mandatory but I fail to see what we gain 
> in making it expand by default in future.

Less ugly syntax, in 99.9% of usages, you would want expand=True and
getVar("X") is neater than getVar("X", True).

Cheers,

Richard

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Martin Jansa]

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

On Wed, Feb 03, 2016 at 11:22:36AM +0000, Richard Purdie wrote:
> On Wed, 2016-02-03 at 08:11 -0200, Otavio Salvador wrote:
> > On Tue, Feb 2, 2016 at 9:55 PM, Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > > At some point in the future, getVar should expand by default. To
> > > get
> > > there from the current position, we need a period of time where the
> > > expand parameter is mandatory.
> > > 
> > > This patch starts that process. Clear errors will result from any
> > > code
> > > which doesn't provide this. Layers can be fixed with an expression
> > > like:
> > > 
> > > sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep 
> > > -ril getVar *`
> > > 
> > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > 
> > I am in favor of making it mandatory but I fail to see what we gain 
> > in making it expand by default in future.
> 
> Less ugly syntax, in 99.9% of usages, you would want expand=True and
> getVar("X") is neater than getVar("X", True).

Consistent indentation is also neater than mixing tabs and spaces, but
you were refusing to take such cosmetic change and now you're proposing
neater getVar syntax which is (in some cases silently) breaking existing
metadata and make backporting changes more difficult?

I'm not opposing the change, just don't understand why the same
arguments work here and not for indentation.

Regards,

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

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

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Richard Purdie]

On Wed, 2016-02-03 at 16:06 +0100, Martin Jansa wrote:
> On Wed, Feb 03, 2016 at 11:22:36AM +0000, Richard Purdie wrote:
> > On Wed, 2016-02-03 at 08:11 -0200, Otavio Salvador wrote:
> > > I am in favor of making it mandatory but I fail to see what we
> > > gain 
> > > in making it expand by default in future.
> > 
> > Less ugly syntax, in 99.9% of usages, you would want expand=True
> > and
> > getVar("X") is neater than getVar("X", True).
> 
> Consistent indentation is also neater than mixing tabs and spaces,
> but
> you were refusing to take such cosmetic change and now you're
> proposing
> neater getVar syntax which is (in some cases silently) breaking
> existing
> metadata and make backporting changes more difficult?
> 
> I'm not opposing the change, just don't understand why the same
> arguments work here and not for indentation.

If we go through the full transition, its possible something might
silently break, if they skip the release where we require a value. We
haven't reached that point yet and I'm honestly not sure how long we
should wait, I'm open to suggestions. The aim is to minimise any silent
failures though.

I'd also add that in many cases, it likely won't break at all, it might
potentially fix something. If there are concerns about this, there are
extra things we could consider such as requiring layers to mark they've
been validated for the change. That adds extra complexity, I'm less
sure about whether we need to do that.

As for your other point, whitespace in shell functions doesn't seem to
cause real world usability issues in that most people can read a tab
indented or space indented function equally well. OE-Core is pretty
consistent about what it uses and always has been, you personally just
don't like the particular choice made and meta-oe deciding to do
something else, contra to the TSC doesn't exactly help consistency. You
could argue meta-oe is more of the problem here.

The actual getVar syntax on the other handy is more than cosmetic,
people don't understand what this "True" they keep adding means, or why
they need it. You tend to know when you don't want expansion on the
other hand. So a cleaner syntax which does what the user most likely
needs is more than cosmetic.

Cheers,

Richard

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Christopher Larson]

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

On Wed, Feb 3, 2016 at 9:22 AM, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Wed, 2016-02-03 at 16:06 +0100, Martin Jansa wrote:
> > On Wed, Feb 03, 2016 at 11:22:36AM +0000, Richard Purdie wrote:
> > > On Wed, 2016-02-03 at 08:11 -0200, Otavio Salvador wrote:
> > > > I am in favor of making it mandatory but I fail to see what we
> > > > gain
> > > > in making it expand by default in future.
> > >
> > > Less ugly syntax, in 99.9% of usages, you would want expand=True
> > > and
> > > getVar("X") is neater than getVar("X", True).
> >
> > Consistent indentation is also neater than mixing tabs and spaces,
> > but
> > you were refusing to take such cosmetic change and now you're
> > proposing
> > neater getVar syntax which is (in some cases silently) breaking
> > existing
> > metadata and make backporting changes more difficult?
> >
> > I'm not opposing the change, just don't understand why the same
> > arguments work here and not for indentation.
>
> If we go through the full transition, its possible something might
> silently break, if they skip the release where we require a value. We
> haven't reached that point yet and I'm honestly not sure how long we
> should wait, I'm open to suggestions. The aim is to minimise any silent
> failures though.
>
> I'd also add that in many cases, it likely won't break at all, it might
> potentially fix something. If there are concerns about this, there are
> extra things we could consider such as requiring layers to mark they've
> been validated for the change. That adds extra complexity, I'm less
> sure about whether we need to do that.
>
> As for your other point, whitespace in shell functions doesn't seem to
> cause real world usability issues in that most people can read a tab
> indented or space indented function equally well. OE-Core is pretty
> consistent about what it uses and always has been, you personally just
> don't like the particular choice made and meta-oe deciding to do
> something else, contra to the TSC doesn't exactly help consistency. You
> could argue meta-oe is more of the problem here.
>

For what it's worth, I use consistent indentation deviating from oe-core in
every layer I maintain as well. Anything else is more hassle than its worth.


> The actual getVar syntax on the other handy is more than cosmetic,
> people don't understand what this "True" they keep adding means, or why
> they need it. You tend to know when you don't want expansion on the
> other hand. So a cleaner syntax which does what the user most likely
> needs is more than cosmetic.


I'd agree with that, passing booleans as arguments sucks all around, since
it loses context. *If* someone is going to do it, it's best to pass it with
the argument name in keyword form in the caller, and obviously we don't
want to do that here.

That said, I'm rather inclined toward thinking about alternative DataSmart
APIs in general, in the long term. For example, it's a MutableMapping, so
it acts dict-like, so we should be able to use .get() and get an expanded
value, and then the case would line up with PEP8 (CamelCase for functions
is frowned upon). I.e. we could have get() and get_unexpanded().
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 4306 bytes --]

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Richard Purdie]

On Wed, 2016-02-03 at 09:29 -0700, Christopher Larson wrote:
> >  The actual getVar syntax on the other handy is more than cosmetic,
> > people don't understand what this "True" they keep adding means, or
> > why
> > they need it. You tend to know when you don't want expansion on the
> > other hand. So a cleaner syntax which does what the user most
> > likely
> > needs is more than cosmetic.
> I'd agree with that, passing booleans as arguments sucks all around,
> since it loses context. *If* someone is going to do it, it's best to
> pass it with the argument name in keyword form in the caller, and
> obviously we don't want to do that here.
> 
> That said, I'm rather inclined toward thinking about alternative
> DataSmart APIs in general, in the long term. For example, it's a
> MutableMapping, so it acts dict-like, so we should be able to use
> .get() and get an expanded value, and then the case would line up
> with PEP8 (CamelCase for functions is frowned upon). I.e. we could
> have get() and get_unexpanded().

I'm just thinking out loud here, I think the above is worth seriously
considering. One big advantage "getVar" has is that its pretty unique
and the python parsing code therefore doesn't get a lot of false
positives with function names. "get" on the other hand, I can see being
a nightmare.

Cheers,

Richard

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Christopher Larson]

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

On Wed, Feb 3, 2016 at 9:38 AM, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> On Wed, 2016-02-03 at 09:29 -0700, Christopher Larson wrote:
> > >  The actual getVar syntax on the other handy is more than cosmetic,
> > > people don't understand what this "True" they keep adding means, or
> > > why
> > > they need it. You tend to know when you don't want expansion on the
> > > other hand. So a cleaner syntax which does what the user most
> > > likely
> > > needs is more than cosmetic.
> > I'd agree with that, passing booleans as arguments sucks all around,
> > since it loses context. *If* someone is going to do it, it's best to
> > pass it with the argument name in keyword form in the caller, and
> > obviously we don't want to do that here.
> >
> > That said, I'm rather inclined toward thinking about alternative
> > DataSmart APIs in general, in the long term. For example, it's a
> > MutableMapping, so it acts dict-like, so we should be able to use
> > .get() and get an expanded value, and then the case would line up
> > with PEP8 (CamelCase for functions is frowned upon). I.e. we could
> > have get() and get_unexpanded().
>
> I'm just thinking out loud here, I think the above is worth seriously
> considering. One big advantage "getVar" has is that its pretty unique
> and the python parsing code therefore doesn't get a lot of false
> positives with function names. "get" on the other hand, I can see being
> a nightmare.


That's a good point, I'll admit I didn't think about the codeparsing in
that regard. I wonder how much work it'd be to track what variable(s) hold
DataSmart instances.. we know 'd' starts that way, that's an assumption we
can make, but it wouldn't traverse createCopy().. *Ponders*
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 2555 bytes --]

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Otavio Salvador]

On Wed, Feb 3, 2016 at 1:06 PM, Martin Jansa <martin.jansa@gmail.com> wrote:
> On Wed, Feb 03, 2016 at 11:22:36AM +0000, Richard Purdie wrote:
>> On Wed, 2016-02-03 at 08:11 -0200, Otavio Salvador wrote:
>> > On Tue, Feb 2, 2016 at 9:55 PM, Richard Purdie
>> > <richard.purdie@linuxfoundation.org> wrote:
>> > > At some point in the future, getVar should expand by default. To
>> > > get
>> > > there from the current position, we need a period of time where the
>> > > expand parameter is mandatory.
>> > >
>> > > This patch starts that process. Clear errors will result from any
>> > > code
>> > > which doesn't provide this. Layers can be fixed with an expression
>> > > like:
>> > >
>> > > sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep
>> > > -ril getVar *`
>> > >
>> > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
>> >
>> > I am in favor of making it mandatory but I fail to see what we gain
>> > in making it expand by default in future.
>>
>> Less ugly syntax, in 99.9% of usages, you would want expand=True and
>> getVar("X") is neater than getVar("X", True).
>
> Consistent indentation is also neater than mixing tabs and spaces, but
> you were refusing to take such cosmetic change and now you're proposing
> neater getVar syntax which is (in some cases silently) breaking existing
> metadata and make backporting changes more difficult?
>
> I'm not opposing the change, just don't understand why the same
> arguments work here and not for indentation.

I second Martin's comments here. I would like to see more consistency
applied to the metadata. I know that some people will have conflicts
when rebasing out-of-tree code but this is the price of not
upstreaming code.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Paul Eggleton]

On Tue, 02 Feb 2016 23:55:40 Richard Purdie wrote:
> At some point in the future, getVar should expand by default. To get
> there from the current position, we need a period of time where the
> expand parameter is mandatory.
> 
> This patch starts that process. Clear errors will result from any code
> which doesn't provide this. Layers can be fixed with an expression
> like:
> 
> sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep -ril getVar
> *`
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> diff --git a/bitbake/lib/bb/data_smart.py b/bitbake/lib/bb/data_smart.py
> index 8c3e9ef..79bec1b 100644
> --- a/bitbake/lib/bb/data_smart.py
> +++ b/bitbake/lib/bb/data_smart.py
> @@ -566,7 +566,7 @@ class DataSmart(MutableMapping):
>                  if len(shortvar) == 0:
>                      override = None
> 
> -    def getVar(self, var, expand=False, noweakdefault=False,
> parsing=False): +    def getVar(self, var, expand, noweakdefault=False,
> parsing=False): return self.getVarFlag(var, "_content", expand,
> noweakdefault, parsing)
> 
>      def renameVar(self, key, newkey, **loginfo):

This change is reasonable and we've been discussing it on and off for years 
now, so I'm generally in favour.

However, what I would really like to see us do is write a script that people 
can use to upgrade their layers to accommodate this and any other breaking 
changes so they don't have to go and figure out what to do themselves; the more 
of these kinds of changes we make the more useful this script will be. I'll 
file a bug.

Cheers,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre

Re: [Openembedded-architecture] [PATCH] data_smart: Drop default expand=False to getVar [API change]

[Richard Purdie]

On Sat, 2016-02-06 at 19:38 +1300, Paul Eggleton wrote:
> On Tue, 02 Feb 2016 23:55:40 Richard Purdie wrote:
> > At some point in the future, getVar should expand by default. To
> > get
> > there from the current position, we need a period of time where the
> > expand parameter is mandatory.
> > 
> > This patch starts that process. Clear errors will result from any
> > code
> > which doesn't provide this. Layers can be fixed with an expression
> > like:
> > 
> > sed -e 's:\(\.getVar([^,()]*, [^,()]*\)):\1, False):g' -i `grep 
> > -ril getVar
> > *`
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > 
> > diff --git a/bitbake/lib/bb/data_smart.py
> > b/bitbake/lib/bb/data_smart.py
> > index 8c3e9ef..79bec1b 100644
> > --- a/bitbake/lib/bb/data_smart.py
> > +++ b/bitbake/lib/bb/data_smart.py
> > @@ -566,7 +566,7 @@ class DataSmart(MutableMapping):
> >                  if len(shortvar) == 0:
> >                      override = None
> > 
> > -    def getVar(self, var, expand=False, noweakdefault=False,
> > parsing=False): +    def getVar(self, var, expand,
> > noweakdefault=False,
> > parsing=False): return self.getVarFlag(var, "_content", expand,
> > noweakdefault, parsing)
> > 
> >      def renameVar(self, key, newkey, **loginfo):
> 
> This change is reasonable and we've been discussing it on and off for
> years 
> now, so I'm generally in favour.
> 
> However, what I would really like to see us do is write a script that
> people 
> can use to upgrade their layers to accommodate this and any other
> breaking 
> changes so they don't have to go and figure out what to do
> themselves; the more 
> of these kinds of changes we make the more useful this script will
> be. I'll 
> file a bug.

This sounds great at first glance. What are the expectations here
though?

The getVar/getVarFlag change can be automatic, however, the MAKEFLAGS
change can't, nor can the python expansion change, at least not at all
easily. I suspect that this change is unusual and that in most cases we
make changes which can't be automatically fixed.

I worry that the user expectation then changes that we'd only ever make
changes which the script can 'save' them from and we end up blocked
from making any other kind of change. Some could argue that is a good
thing, I'm less sure.

This is one reason I put the sed command in these commits. I would note
that the version in one that I sent to the list was incorrect, I did
fix it on the branch though.

The compromise might be better migration notes in the metadata for
these issues?

[Incidentally, there are other kinds of breakage, e.g. I just fixed one
of the sanity tests and merged the fix, thinking I'd checked the
metadata for issues. Turns out multilib is still broken :(. Fixing that
breakage is way beyond a script, sadly, it will probably take me most
of the weekend to untangle that mess :(]

Cheers,

Richard