Patchwork [bitbake-devel] PLEASE READ: Major change landing shortly (python whitespace)

login
register
mail settings
Submitter Richard Purdie
Date July 18, 2012, 10:06 a.m.
Message ID <1342606005.30680.17.camel@ted>
Download mbox | patch
Permalink /patch/32405/
State New
Headers show

Comments

Richard Purdie - July 18, 2012, 10:06 a.m.
It's become clear we have a horrible mixture of whitespace (tabs and
space) in python functions. At first glance this sounds trivial but
there is a serious problem if pieces of code get mixed together with
Martin Jansa - July 18, 2012, 10:40 a.m.
On Wed, Jul 18, 2012 at 11:06:45AM +0100, Richard Purdie wrote:
> It's become clear we have a horrible mixture of whitespace (tabs and
> space) in python functions. At first glance this sounds trivial but
> there is a serious problem if pieces of code get mixed together with
> different whitespace since they may or may not work as intended.
> 
> The documented standard for python functions is four space indentation
> although we have a mixture. Fixing this sounds simple at first, we just
> go through and change tabs to spaces. The problem comes with _append and
> _prepend to python functions since both need their whitespace changed at
> the same time. populate_packages() is the real problem child in that
> regard but its the less common examples I worry about.
> 
> I did some research and we have inconsistencies, even in existing
> functions since people are editing files and getting confused
> (understandably).
> 
> So the question becomes, how do we get ourselves out of this mess?
> 
> I put a proposal to the TSC, that we have bitbake warn/error whenever it
> finds tab characters in any python function. The advantage of this is
> that we give the user a clear definitive error. The downside is that
> we'll have to go through all the metadata and scrub it for the problem.
> 
> The TSC agrees we should do something about this rather than perpetuate
> the problem and that this is as good a solution as any of us can come up
> with. We also agreed that we should do it sooner than later before it
> gets any later in this release cycle. Putting it off until the next
> release cycle didn't seem to offer any advantage, we might as well get
> on with it.
> 
> So I am going to:
> 
> a) Try and flush through as many pending patches as I can.
> b) Check in a warning into bitbake master and increase its version
> c) Require that version in OE-Core

Great.. this is usefull also for latest change from proto to protocol
param in fetcher, but please make sure that the svn.py patch I've sent
yesterday is also in before release..

> d) Commit a significant set of whitespace changes to OE-Core, resolving
> all the warnings for OE-Core.
> 
> I plan to do this, tomorrow, Thursday.
> 
> The reason for getting on with it is that the patches are horrible to
> carry around and that any other patches made against OE-Core will likely
> need to be remade to apply after these changes. I therefore don't want a
> week of waiting around discussing it, this is something I believe we
> need to do now and be done with it. I'm also not going to post the
> whitespace patches for review, I'll write+merge them and then be
> responsive for accepting fixes for anything that gets broken. They will
> only be whitespace changes.
> 
> I appreciate this is going to cause some disruption but I think there is
> a problem worth solving here and this is the best way to do it. If
> anyone has any strong objections I'm open to alternative ideas.
> 
> The bitbake patch to error on tab whitespace is included below. I'll try
> and post a branch with the whitespace fixes on later today.
> 
> Cheers,
> 
> Richard
> 
> 
> 
> 
> diff --git a/bitbake/lib/bb/data.py b/bitbake/lib/bb/data.py
> index e3ffefe..4b653a5 100644
> --- a/bitbake/lib/bb/data.py
> +++ b/bitbake/lib/bb/data.py
> @@ -291,6 +291,8 @@ def build_dependencies(key, keys, shelldeps, vardepvals, d):
>              if d.getVarFlag(key, "python"):
>                  parsedvar = d.expandWithRefs(value, key)
>                  parser = bb.codeparser.PythonParser(key, logger)
> +                if parsedvar.value and "\t" in parsedvar.value:
> +                    bb.warn("Variable %s contains tabs, please remove these" % key)
>                  parser.parse_python(parsedvar.value)
>                  deps = deps | parser.references
>              else:
> diff --git a/bitbake/lib/bb/parse/ast.py b/bitbake/lib/bb/parse/ast.py
> index eae840f..86f9463 100644
> --- a/bitbake/lib/bb/parse/ast.py
> +++ b/bitbake/lib/bb/parse/ast.py
> @@ -212,9 +212,9 @@ class ExportFuncsNode(AstNode):
>                          data.setVarFlag(calledvar, flag, data.getVarFlag(var, flag))
>  
>                  if data.getVarFlag(calledvar, "python"):
> -                    data.setVar(var, "\tbb.build.exec_func('" + calledvar + "', d)\n")
> +                    data.setVar(var, "    bb.build.exec_func('" + calledvar + "', d)\n")
>                  else:
> -                    data.setVar(var, "\t" + calledvar + "\n")
> +                    data.setVar(var, "    " + calledvar + "\n")
>                  data.setVarFlag(var, 'export_func', '1')
>  
>  class AddTaskNode(AstNode):
> 
> 
> 
> _______________________________________________
> bitbake-devel mailing list
> bitbake-devel@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/bitbake-devel
Richard Purdie - July 18, 2012, 11:40 a.m.
On Wed, 2012-07-18 at 11:17 +0100, Burton, Ross wrote:
> On 18 July 2012 11:06, Richard Purdie
> <richard.purdie@linuxfoundation.org> wrote:
> > I put a proposal to the TSC, that we have bitbake warn/error whenever it
> > finds tab characters in any python function. The advantage of this is
> > that we give the user a clear definitive error. The downside is that
> > we'll have to go through all the metadata and scrub it for the problem.
> 
> Have you ran that warning over oe-core to check that there are not any
> legitimate uses of \t, not for indentation but inside strings?  I
> can't think of any realistic use but you never know (construct a
> Makefile in a python function?).

The check is for actual tab characters, not "\t". There are some
legitimate users of tab characters which I've replaced with \t in
strings.

My current patch work in progress for the conversion is:

http://git.yoctoproject.org/cgit.cgi/poky-contrib/commit/?h=rpurdie/t14&id=49d3d01f3d61a0eb19b6852229fa8fc26712f653

Cheers,

Richard
Martin Jansa - July 19, 2012, 9:49 a.m.
On Wed, Jul 18, 2012 at 12:40:18PM +0100, Richard Purdie wrote:
> On Wed, 2012-07-18 at 11:17 +0100, Burton, Ross wrote:
> > On 18 July 2012 11:06, Richard Purdie
> > <richard.purdie@linuxfoundation.org> wrote:
> > > I put a proposal to the TSC, that we have bitbake warn/error whenever it
> > > finds tab characters in any python function. The advantage of this is
> > > that we give the user a clear definitive error. The downside is that
> > > we'll have to go through all the metadata and scrub it for the problem.
> > 
> > Have you ran that warning over oe-core to check that there are not any
> > legitimate uses of \t, not for indentation but inside strings?  I
> > can't think of any realistic use but you never know (construct a
> > Makefile in a python function?).
> 
> The check is for actual tab characters, not "\t". There are some
> legitimate users of tab characters which I've replaced with \t in
> strings.
> 
> My current patch work in progress for the conversion is:
> 
> http://git.yoctoproject.org/cgit.cgi/poky-contrib/commit/?h=rpurdie/t14&id=49d3d01f3d61a0eb19b6852229fa8fc26712f653

from those 2 patches which were just merged I see that you're converting
strictly python functions, can we extend this tabs->spaces rule also to
bash tasks like do_install etc?

Cheers,
Richard Purdie - July 19, 2012, 10:15 a.m.
On Thu, 2012-07-19 at 11:49 +0200, Martin Jansa wrote:
> On Wed, Jul 18, 2012 at 12:40:18PM +0100, Richard Purdie wrote:
> > On Wed, 2012-07-18 at 11:17 +0100, Burton, Ross wrote:
> > > On 18 July 2012 11:06, Richard Purdie
> > > <richard.purdie@linuxfoundation.org> wrote:
> > > > I put a proposal to the TSC, that we have bitbake warn/error whenever it
> > > > finds tab characters in any python function. The advantage of this is
> > > > that we give the user a clear definitive error. The downside is that
> > > > we'll have to go through all the metadata and scrub it for the problem.
> > > 
> > > Have you ran that warning over oe-core to check that there are not any
> > > legitimate uses of \t, not for indentation but inside strings?  I
> > > can't think of any realistic use but you never know (construct a
> > > Makefile in a python function?).
> > 
> > The check is for actual tab characters, not "\t". There are some
> > legitimate users of tab characters which I've replaced with \t in
> > strings.
> > 
> > My current patch work in progress for the conversion is:
> > 
> > http://git.yoctoproject.org/cgit.cgi/poky-contrib/commit/?h=rpurdie/t14&id=49d3d01f3d61a0eb19b6852229fa8fc26712f653
> 
> from those 2 patches which were just merged I see that you're converting
> strictly python functions, can we extend this tabs->spaces rule also to
> bash tasks like do_install etc?

Shell tasks should be tabs according to the style guide. Its harder to
check the indentation in those and if the indentation is wrong, it
doesn't matter since they're not whitespace sensitive.

So whilst I'd welcome fixing them up, I don't think they need bitbake
enforcing policy in the same way as python functions.

Cheers,

Richard
Martin Jansa - July 19, 2012, 11:18 a.m.
On Thu, Jul 19, 2012 at 11:15:48AM +0100, Richard Purdie wrote:
> On Thu, 2012-07-19 at 11:49 +0200, Martin Jansa wrote:
> > On Wed, Jul 18, 2012 at 12:40:18PM +0100, Richard Purdie wrote:
> > > On Wed, 2012-07-18 at 11:17 +0100, Burton, Ross wrote:
> > > > On 18 July 2012 11:06, Richard Purdie
> > > > <richard.purdie@linuxfoundation.org> wrote:
> > > > > I put a proposal to the TSC, that we have bitbake warn/error whenever it
> > > > > finds tab characters in any python function. The advantage of this is
> > > > > that we give the user a clear definitive error. The downside is that
> > > > > we'll have to go through all the metadata and scrub it for the problem.
> > > > 
> > > > Have you ran that warning over oe-core to check that there are not any
> > > > legitimate uses of \t, not for indentation but inside strings?  I
> > > > can't think of any realistic use but you never know (construct a
> > > > Makefile in a python function?).
> > > 
> > > The check is for actual tab characters, not "\t". There are some
> > > legitimate users of tab characters which I've replaced with \t in
> > > strings.
> > > 
> > > My current patch work in progress for the conversion is:
> > > 
> > > http://git.yoctoproject.org/cgit.cgi/poky-contrib/commit/?h=rpurdie/t14&id=49d3d01f3d61a0eb19b6852229fa8fc26712f653
> > 
> > from those 2 patches which were just merged I see that you're converting
> > strictly python functions, can we extend this tabs->spaces rule also to
> > bash tasks like do_install etc?
> 
> Shell tasks should be tabs according to the style guide. Its harder to
> check the indentation in those and if the indentation is wrong, it
> doesn't matter since they're not whitespace sensitive.
> 
> So whilst I'd welcome fixing them up, I don't think they need bitbake
> enforcing policy in the same way as python functions.

Agreed about not forcing the check or updating all .bb/.inc files with
this now, but maybe style guide should be updated now?

I really don't like files with mixed indentation (even when it doesn't
hurt) especially with a lot of files not conforming to style guide now,
using even mix of tabs/spaces on the same line..

FWIW: I've prepared patch for whole meta-smartphone to unify that and
I've sent RFC for few bbclasses in meta-oe too.

Cheers,
Martin Jansa - July 19, 2012, 11:27 a.m.
On Thu, Jul 19, 2012 at 01:18:12PM +0200, Martin Jansa wrote:
> On Thu, Jul 19, 2012 at 11:15:48AM +0100, Richard Purdie wrote:
> > On Thu, 2012-07-19 at 11:49 +0200, Martin Jansa wrote:
> > > On Wed, Jul 18, 2012 at 12:40:18PM +0100, Richard Purdie wrote:
> > > > On Wed, 2012-07-18 at 11:17 +0100, Burton, Ross wrote:
> > > > > On 18 July 2012 11:06, Richard Purdie
> > > > > <richard.purdie@linuxfoundation.org> wrote:
> > > > > > I put a proposal to the TSC, that we have bitbake warn/error whenever it
> > > > > > finds tab characters in any python function. The advantage of this is
> > > > > > that we give the user a clear definitive error. The downside is that
> > > > > > we'll have to go through all the metadata and scrub it for the problem.
> > > > > 
> > > > > Have you ran that warning over oe-core to check that there are not any
> > > > > legitimate uses of \t, not for indentation but inside strings?  I
> > > > > can't think of any realistic use but you never know (construct a
> > > > > Makefile in a python function?).
> > > > 
> > > > The check is for actual tab characters, not "\t". There are some
> > > > legitimate users of tab characters which I've replaced with \t in
> > > > strings.
> > > > 
> > > > My current patch work in progress for the conversion is:
> > > > 
> > > > http://git.yoctoproject.org/cgit.cgi/poky-contrib/commit/?h=rpurdie/t14&id=49d3d01f3d61a0eb19b6852229fa8fc26712f653
> > > 
> > > from those 2 patches which were just merged I see that you're converting
> > > strictly python functions, can we extend this tabs->spaces rule also to
> > > bash tasks like do_install etc?
> > 
> > Shell tasks should be tabs according to the style guide. Its harder to
> > check the indentation in those and if the indentation is wrong, it
> > doesn't matter since they're not whitespace sensitive.
> > 
> > So whilst I'd welcome fixing them up, I don't think they need bitbake
> > enforcing policy in the same way as python functions.
> 
> Agreed about not forcing the check or updating all .bb/.inc files with
> this now, but maybe style guide should be updated now?
> 
> I really don't like files with mixed indentation (even when it doesn't
> hurt) especially with a lot of files not conforming to style guide now,
> using even mix of tabs/spaces on the same line..
> 
> FWIW: I've prepared patch for whole meta-smartphone to unify that and
> I've sent RFC for few bbclasses in meta-oe too.

I know you've sent me link do different style guide last time I've
asked, but first google hit (for openembedded style guide):
http://www.openembedded.org/wiki/Styleguide

says about tabs only this:
- Use spaces for indentation as developers tends to use different amount
  of spaces per one tab.

So only in yocto wiki there are 2 more bullets after that
https://wiki.yoctoproject.org/wiki/Recipe_%26_Patch_Style_Guide
- Use spaces for indentation as developers tends to use different amount
  of spaces per one tab.
- Shell functions should use tabs
- Python functions should use spaces (4 spaces per indent).

And yocto style guide is not in first 5 pages of result when searching
for "openembedded style guide".

Cheers,

Patch

different whitespace since they may or may not work as intended.

The documented standard for python functions is four space indentation
although we have a mixture. Fixing this sounds simple at first, we just
go through and change tabs to spaces. The problem comes with _append and
_prepend to python functions since both need their whitespace changed at
the same time. populate_packages() is the real problem child in that
regard but its the less common examples I worry about.

I did some research and we have inconsistencies, even in existing
functions since people are editing files and getting confused
(understandably).

So the question becomes, how do we get ourselves out of this mess?

I put a proposal to the TSC, that we have bitbake warn/error whenever it
finds tab characters in any python function. The advantage of this is
that we give the user a clear definitive error. The downside is that
we'll have to go through all the metadata and scrub it for the problem.

The TSC agrees we should do something about this rather than perpetuate
the problem and that this is as good a solution as any of us can come up
with. We also agreed that we should do it sooner than later before it
gets any later in this release cycle. Putting it off until the next
release cycle didn't seem to offer any advantage, we might as well get
on with it.

So I am going to:

a) Try and flush through as many pending patches as I can.
b) Check in a warning into bitbake master and increase its version
c) Require that version in OE-Core
d) Commit a significant set of whitespace changes to OE-Core, resolving
all the warnings for OE-Core.

I plan to do this, tomorrow, Thursday.

The reason for getting on with it is that the patches are horrible to
carry around and that any other patches made against OE-Core will likely
need to be remade to apply after these changes. I therefore don't want a
week of waiting around discussing it, this is something I believe we
need to do now and be done with it. I'm also not going to post the
whitespace patches for review, I'll write+merge them and then be
responsive for accepting fixes for anything that gets broken. They will
only be whitespace changes.

I appreciate this is going to cause some disruption but I think there is
a problem worth solving here and this is the best way to do it. If
anyone has any strong objections I'm open to alternative ideas.

The bitbake patch to error on tab whitespace is included below. I'll try
and post a branch with the whitespace fixes on later today.

Cheers,

Richard




diff --git a/bitbake/lib/bb/data.py b/bitbake/lib/bb/data.py
index e3ffefe..4b653a5 100644
--- a/bitbake/lib/bb/data.py
+++ b/bitbake/lib/bb/data.py
@@ -291,6 +291,8 @@  def build_dependencies(key, keys, shelldeps, vardepvals, d):
             if d.getVarFlag(key, "python"):
                 parsedvar = d.expandWithRefs(value, key)
                 parser = bb.codeparser.PythonParser(key, logger)
+                if parsedvar.value and "\t" in parsedvar.value:
+                    bb.warn("Variable %s contains tabs, please remove these" % key)
                 parser.parse_python(parsedvar.value)
                 deps = deps | parser.references
             else:
diff --git a/bitbake/lib/bb/parse/ast.py b/bitbake/lib/bb/parse/ast.py
index eae840f..86f9463 100644
--- a/bitbake/lib/bb/parse/ast.py
+++ b/bitbake/lib/bb/parse/ast.py
@@ -212,9 +212,9 @@  class ExportFuncsNode(AstNode):
                         data.setVarFlag(calledvar, flag, data.getVarFlag(var, flag))
 
                 if data.getVarFlag(calledvar, "python"):
-                    data.setVar(var, "\tbb.build.exec_func('" + calledvar + "', d)\n")
+                    data.setVar(var, "    bb.build.exec_func('" + calledvar + "', d)\n")
                 else:
-                    data.setVar(var, "\t" + calledvar + "\n")
+                    data.setVar(var, "    " + calledvar + "\n")
                 data.setVarFlag(var, 'export_func', '1')
 
 class AddTaskNode(AstNode):