Patchwork [Bitbake-dev,RFC] bitbake/BBHandler: Improve handling of multiline comments and warn users of the change

login
register
mail settings
Submitter Richard Purdie
Date Feb. 23, 2011, 4:51 p.m.
Message ID <1298479894.26736.51.camel@rex>
Download mbox | patch
Permalink /patch/789/
State New
Headers show

Comments

Richard Purdie - Feb. 23, 2011, 4:51 p.m.
The behaviour of "#" in multiline variables is established but very
confusing for people and can cause data to disappear unexpectedly.

Currently this construct would drop one of the patches:

SRC_URI = "http://z/ \
          # file://xxx.patch \
           file://yyy.patch"

which in that context makes sense and people do use this. On the other
hand people have done things like:

VARIABLE = "Some text which references the C code: \
            #include "config.h" \
            where we wanted to make some comment"

and then wondered why the line went missing. Since this is done
silently, it can be hard to even realise there was a problem.

I've applied a patch in Poky which triggers big warnings about doing
that and also changed the behaviour to include the line instead of
dropping it. I'm not sure whether this change is desirable or not. It
would certainly improve user experience if we didn't special case
commends in multi line strings as its not what people would expect
having used any other language.

Feedback welcome...

Cheers,

Richard



bitbake/BBHandler: Improve handling of multiline comments and warn users of the change

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Khem Raj - March 1, 2011, 2:53 a.m.
On (23/02/11 16:51), Richard Purdie wrote:
> The behaviour of "#" in multiline variables is established but very
> confusing for people and can cause data to disappear unexpectedly.
> 
> Currently this construct would drop one of the patches:
> 
> SRC_URI = "http://z/ \
>           # file://xxx.patch \
>            file://yyy.patch"
> 
> which in that context makes sense and people do use this. On the other
> hand people have done things like:
> 
> VARIABLE = "Some text which references the C code: \
>             #include "config.h" \
>             where we wanted to make some comment"
> 
> and then wondered why the line went missing. Since this is done
> silently, it can be hard to even realise there was a problem.
> 
> I've applied a patch in Poky which triggers big warnings about doing
> that and also changed the behaviour to include the line instead of
> dropping it. I'm not sure whether this change is desirable or not. It
> would certainly improve user experience if we didn't special case
> commends in multi line strings as its not what people would expect
> having used any other language.
> 
> Feedback welcome...

would it make sense to treat file:// http:// patch:// etc (SRC_URIs) specially ? and treat # before it
as comment but not otherwise and we may ask to escape special
characters like # inside strings if they are not supposed to be
interpreted by bitbake. If # appears in string and is not above case
then we warn about it ?

> 
> Cheers,
> 
> Richard
> 
> 
> 
> bitbake/BBHandler: Improve handling of multiline comments and warn users of the change
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> 
> diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> index 31d1e21..402cd07 100644
> --- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
> +++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
> @@ -193,9 +193,14 @@ def feeder(lineno, s, fn, root, statements):
> if lineno == IN_PYTHON_EOF:
> return
> 
> -# fall through
> 
> - if s == '' or s[0] == '#': return # skip comments and empty lines
> + # Skip empty lines
> + if s == '':
> + return 
> +
> + if s[0] == '#':
> + if len(__residue__) != 0 and __residue__[0][0] != "#":
> + bb.error("There is a comment on line %s of file %s (%s) which is in the middle of a multiline expression.\nBitbake used to ignore these but no longer does so, please fix your metadata as errors are likely as a result of this change." % (lineno, fn, s))
> 
> if s[-1] == '\\':
> __residue__.append(s[:-1])
> @@ -204,6 +209,10 @@ def feeder(lineno, s, fn, root, statements):
> s = "".join(__residue__) + s
> __residue__ = []
> 
> + # Skip comments
> + if s[0] == '#':
> + return
> +
> m = __func_start_regexp__.match(s)
> if m:
> __infunc__ = m.group("func") or "__anonymous"
> 
> 
> _______________________________________________
> Bitbake-dev mailing list
> Bitbake-dev@lists.berlios.de
> https://lists.berlios.de/mailman/listinfo/bitbake-dev
Esben Haabendal - March 1, 2011, 11:47 a.m.
On Mon, 2011-02-28 at 18:53 -0800, Khem Raj wrote:
> On (23/02/11 16:51), Richard Purdie wrote:
> > The behaviour of "#" in multiline variables is established but very
> > confusing for people and can cause data to disappear unexpectedly.
> > 
> > Currently this construct would drop one of the patches:
> > 
> > SRC_URI = "http://z/ \
> >           # file://xxx.patch \
> >            file://yyy.patch"
> > 
> > which in that context makes sense and people do use this. On the other
> > hand people have done things like:
> > 
> > VARIABLE = "Some text which references the C code: \
> >             #include "config.h" \
> >             where we wanted to make some comment"
> > 
> > and then wondered why the line went missing. Since this is done
> > silently, it can be hard to even realise there was a problem.
> > 
> > I've applied a patch in Poky which triggers big warnings about doing
> > that and also changed the behaviour to include the line instead of
> > dropping it. I'm not sure whether this change is desirable or not. It
> > would certainly improve user experience if we didn't special case
> > commends in multi line strings as its not what people would expect
> > having used any other language.
> > 
> > Feedback welcome...
> 
> would it make sense to treat file:// http:// patch:// etc (SRC_URIs) specially ? and treat # before it
> as comment but not otherwise and we may ask to escape special
> characters like # inside strings if they are not supposed to be
> interpreted by bitbake. If # appears in string and is not above case
> then we warn about it ?

I think it might be worth the effort to try and apply a bit of
"Principle of Least Astonishment" [1].  Having different handling of
characters in strings depending on which variables they are used in
certainly would seem surprising to some.

  [1] http://en.wikipedia.org/wiki/Principle_of_least_astonishment

Going down that path, how would you handle use-cases where a variable is
appended to SRC_URI.  Which '#' handling should be applied to it then?
If the special SRC_URI handling should be applied, should the same
variable then result in a different value when applied to other (than
SRC_URI) variables?

Let's just stick to one way.

/Esben
Richard Purdie - March 3, 2011, 3:54 p.m.
On Tue, 2011-03-01 at 12:47 +0100, Esben Haabendal wrote:
> On Mon, 2011-02-28 at 18:53 -0800, Khem Raj wrote:
> > On (23/02/11 16:51), Richard Purdie wrote:
> > > The behaviour of "#" in multiline variables is established but very
> > > confusing for people and can cause data to disappear unexpectedly.
> > > 
> > > Currently this construct would drop one of the patches:
> > > 
> > > SRC_URI = "http://z/ \
> > >           # file://xxx.patch \
> > >            file://yyy.patch"
> > > 
> > > which in that context makes sense and people do use this. On the other
> > > hand people have done things like:
> > > 
> > > VARIABLE = "Some text which references the C code: \
> > >             #include "config.h" \
> > >             where we wanted to make some comment"
> > > 
> > > and then wondered why the line went missing. Since this is done
> > > silently, it can be hard to even realise there was a problem.
> > > 
> > > I've applied a patch in Poky which triggers big warnings about doing
> > > that and also changed the behaviour to include the line instead of
> > > dropping it. I'm not sure whether this change is desirable or not. It
> > > would certainly improve user experience if we didn't special case
> > > commends in multi line strings as its not what people would expect
> > > having used any other language.
> > > 
> > > Feedback welcome...
> > 
> > would it make sense to treat file:// http:// patch:// etc (SRC_URIs) specially ? and treat # before it
> > as comment but not otherwise and we may ask to escape special
> > characters like # inside strings if they are not supposed to be
> > interpreted by bitbake. If # appears in string and is not above case
> > then we warn about it ?
> 
> I think it might be worth the effort to try and apply a bit of
> "Principle of Least Astonishment" [1].  Having different handling of
> characters in strings depending on which variables they are used in
> certainly would seem surprising to some.
> 
>   [1] http://en.wikipedia.org/wiki/Principle_of_least_astonishment
> 
> Going down that path, how would you handle use-cases where a variable is
> appended to SRC_URI.  Which '#' handling should be applied to it then?
> If the special SRC_URI handling should be applied, should the same
> variable then result in a different value when applied to other (than
> SRC_URI) variables?
> 
> Let's just stick to one way.

I agree, either we accept comments in multiline variables or we don't
but making it conditional upon some context is just going to make things
worse.

Cheers,

Richard
Chris Larson - March 3, 2011, 4:16 p.m.
On Thu, Mar 3, 2011 at 8:54 AM, Richard Purdie <rpurdie@rpsys.net> wrote:
> I agree, either we accept comments in multiline variables or we don't
> but making it conditional upon some context is just going to make things
> worse.
>

Agreed.

Patch

diff --git a/bitbake/lib/bb/parse/parse_py/BBHandler.py b/bitbake/lib/bb/parse/parse_py/BBHandler.py
index 31d1e21..402cd07 100644
--- a/bitbake/lib/bb/parse/parse_py/BBHandler.py
+++ b/bitbake/lib/bb/parse/parse_py/BBHandler.py
@@ -193,9 +193,14 @@  def feeder(lineno, s, fn, root, statements):
if lineno == IN_PYTHON_EOF:
return

-# fall through

- if s == '' or s[0] == '#': return # skip comments and empty lines
+ # Skip empty lines
+ if s == '':
+ return 
+
+ if s[0] == '#':
+ if len(__residue__) != 0 and __residue__[0][0] != "#":
+ bb.error("There is a comment on line %s of file %s (%s) which is in the middle of a multiline expression.\nBitbake used to ignore these but no longer does so, please fix your metadata as errors are likely as a result of this change." % (lineno, fn, s))

if s[-1] == '\\':
__residue__.append(s[:-1])
@@ -204,6 +209,10 @@  def feeder(lineno, s, fn, root, statements):
s = "".join(__residue__) + s
__residue__ = []

+ # Skip comments
+ if s[0] == '#':
+ return
+
m = __func_start_regexp__.match(s)
if m:
__infunc__ = m.group("func") or "__anonymous"