[PATCH] oe-selftest: Improve BUILDDIR environment handling

[PATCH] oe-selftest: Improve BUILDDIR environment handling

[Richard Purdie]

Its possible something (like bitbake/tinfoil2) may mess around with the
environment and using the enviroment as a global variable store isn't
particularly nice anyway.

This patch changes the BUILDDIR usages so that the environment isn't used
as a global store and a global variable is used instead. Whilst that
is still not perfect, it does avoid the current double and triple backtraces
we're seeing where tinfoil2/bitbake has trampled the enviroment leading
to failures of failures making debugging even harder.

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 scripts/oe-selftest | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/scripts/oe-selftest b/scripts/oe-selftest
index bfcea66..e166521 100755
--- a/scripts/oe-selftest
+++ b/scripts/oe-selftest
@@ -111,9 +111,13 @@ def get_args_parser():
                         help='Submit test results to a repository')
     return parser
 
+builddir = None
+
 
 def preflight_check():
 
+    global builddir
+
     log.info("Checking that everything is in order before running the tests")
 
     if not os.environ.get("BUILDDIR"):
@@ -135,7 +139,7 @@ def preflight_check():
     return True
 
 def add_include():
-    builddir = os.environ.get("BUILDDIR")
+    global builddir
     if "#include added by oe-selftest.py" \
         not in ftools.read_file(os.path.join(builddir, "conf/local.conf")):
             log.info("Adding: \"include selftest.inc\" in local.conf")
@@ -149,7 +153,7 @@ def add_include():
                     "\n#include added by oe-selftest.py\ninclude bblayers.inc")
 
 def remove_include():
-    builddir = os.environ.get("BUILDDIR")
+    global builddir
     if builddir is None:
         return
     if "#include added by oe-selftest.py" \
@@ -165,18 +169,21 @@ def remove_include():
                     "\n#include added by oe-selftest.py\ninclude bblayers.inc")
 
 def remove_inc_files():
+    global builddir
+    if builddir is None:
+        return
     try:
-        os.remove(os.path.join(os.environ.get("BUILDDIR"), "conf/selftest.inc"))
+        os.remove(os.path.join(builddir, "conf/selftest.inc"))
         for root, _, files in os.walk(get_test_layer()):
             for f in files:
                 if f == 'test_recipe.inc':
                     os.remove(os.path.join(root, f))
-    except (AttributeError, OSError,) as e:    # AttributeError may happen if BUILDDIR is not set
+    except OSError as e:
         pass
 
     for incl_file in ['conf/bblayers.inc', 'conf/machine.inc']:
         try:
-            os.remove(os.path.join(os.environ.get("BUILDDIR"), incl_file))
+            os.remove(os.path.join(builddir, incl_file))
         except:
             pass
 
@@ -394,7 +401,7 @@ def coverage_setup(coverage_source, coverage_include, coverage_omit):
     """ Set up the coverage measurement for the testcases to be run """
     import datetime
     import subprocess
-    builddir = os.environ.get("BUILDDIR")
+    global builddir
     pokydir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
     curcommit= subprocess.check_output(["git", "--git-dir", os.path.join(pokydir, ".git"), "rev-parse", "HEAD"]).decode('utf-8')
     coveragerc = "%s/.coveragerc" % builddir
-- 
2.7.4

Re: [PATCH] oe-selftest: Improve BUILDDIR environment handling

[Ed Bartosh]

On Wed, Jan 04, 2017 at 11:48:53PM +0000, Richard Purdie wrote:
> Its possible something (like bitbake/tinfoil2) may mess around with the
> environment and using the enviroment as a global variable store isn't
> particularly nice anyway.
> 
> This patch changes the BUILDDIR usages so that the environment isn't used
> as a global store and a global variable is used instead. Whilst that
> is still not perfect, it does avoid the current double and triple backtraces
> we're seeing where tinfoil2/bitbake has trampled the enviroment leading
> to failures of failures making debugging even harder.
> 
> Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  scripts/oe-selftest | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/oe-selftest b/scripts/oe-selftest
> index bfcea66..e166521 100755
> --- a/scripts/oe-selftest
> +++ b/scripts/oe-selftest
> @@ -111,9 +111,13 @@ def get_args_parser():
>                          help='Submit test results to a repository')
>      return parser
>  
> +builddir = None
> +
>  
>  def preflight_check():
>  
> +    global builddir
> +
>      log.info("Checking that everything is in order before running the tests")
>  
>      if not os.environ.get("BUILDDIR"):
> @@ -135,7 +139,7 @@ def preflight_check():
>      return True
>  
>  def add_include():
> -    builddir = os.environ.get("BUILDDIR")
> +    global builddir
You don't need to use 'global' here. It's only mandatory if
you change variable value:
https://docs.python.org/3/reference/simple_stmts.html#the-global-statement

Would it be more readable to use name in upper case: BUILDDIR?

>      if "#include added by oe-selftest.py" \
>          not in ftools.read_file(os.path.join(builddir, "conf/local.conf")):
>              log.info("Adding: \"include selftest.inc\" in local.conf")
> @@ -149,7 +153,7 @@ def add_include():
>                      "\n#include added by oe-selftest.py\ninclude bblayers.inc")
>  
>  def remove_include():
> -    builddir = os.environ.get("BUILDDIR")
> +    global builddir
>      if builddir is None:
>          return
>      if "#include added by oe-selftest.py" \
> @@ -165,18 +169,21 @@ def remove_include():
>                      "\n#include added by oe-selftest.py\ninclude bblayers.inc")
>  
>  def remove_inc_files():
> +    global builddir
> +    if builddir is None:
> +        return
>      try:
> -        os.remove(os.path.join(os.environ.get("BUILDDIR"), "conf/selftest.inc"))
> +        os.remove(os.path.join(builddir, "conf/selftest.inc"))
>          for root, _, files in os.walk(get_test_layer()):
>              for f in files:
>                  if f == 'test_recipe.inc':
>                      os.remove(os.path.join(root, f))
> -    except (AttributeError, OSError,) as e:    # AttributeError may happen if BUILDDIR is not set
> +    except OSError as e:
>          pass
>  
>      for incl_file in ['conf/bblayers.inc', 'conf/machine.inc']:
>          try:
> -            os.remove(os.path.join(os.environ.get("BUILDDIR"), incl_file))
> +            os.remove(os.path.join(builddir, incl_file))
>          except:
>              pass
>  
> @@ -394,7 +401,7 @@ def coverage_setup(coverage_source, coverage_include, coverage_omit):
>      """ Set up the coverage measurement for the testcases to be run """
>      import datetime
>      import subprocess
> -    builddir = os.environ.get("BUILDDIR")
> +    global builddir
>      pokydir = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
>      curcommit= subprocess.check_output(["git", "--git-dir", os.path.join(pokydir, ".git"), "rev-parse", "HEAD"]).decode('utf-8')
>      coveragerc = "%s/.coveragerc" % builddir
> -- 
> 2.7.4
> 
> -- 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

-- 
--
Regards,
Ed

Re: [PATCH] oe-selftest: Improve BUILDDIR environment handling

[Richard Purdie]

On Thu, 2017-01-05 at 11:40 +0200, Ed Bartosh wrote:
> On Wed, Jan 04, 2017 at 11:48:53PM +0000, Richard Purdie wrote:
> > 
> > Its possible something (like bitbake/tinfoil2) may mess around with
> > the
> > environment and using the enviroment as a global variable store
> > isn't
> > particularly nice anyway.
> > 
> > This patch changes the BUILDDIR usages so that the environment
> > isn't used
> > as a global store and a global variable is used instead. Whilst
> > that
> > is still not perfect, it does avoid the current double and triple
> > backtraces
> > we're seeing where tinfoil2/bitbake has trampled the enviroment
> > leading
> > to failures of failures making debugging even harder.
> > 
> > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > ---
> >  scripts/oe-selftest | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/scripts/oe-selftest b/scripts/oe-selftest
> > index bfcea66..e166521 100755
> > --- a/scripts/oe-selftest
> > +++ b/scripts/oe-selftest
> > @@ -111,9 +111,13 @@ def get_args_parser():
> >                          help='Submit test results to a
> > repository')
> >      return parser
> >  
> > +builddir = None
> > +
> >  
> >  def preflight_check():
> >  
> > +    global builddir
> > +
> >      log.info("Checking that everything is in order before running
> > the tests")
> >  
> >      if not os.environ.get("BUILDDIR"):
> > @@ -135,7 +139,7 @@ def preflight_check():
> >      return True
> >  
> >  def add_include():
> > -    builddir = os.environ.get("BUILDDIR")
> > +    global builddir
> You don't need to use 'global' here. It's only mandatory if
> you change variable value:
> https://docs.python.org/3/reference/simple_stmts.html#the-global-stat
> ement

In this case I did it purely for readability to make it clear how we
were expecting it to work and to match the other uses. It doesn't hurt
anything afaik.

> Would it be more readable to use name in upper case: BUILDDIR?

Not sure, that isn't something we've used as a style anywhere else so
it doesn't really match any other style. Is that a python convention?

Cheers,

Richard

Re: [PATCH] oe-selftest: Improve BUILDDIR environment handling

[Ed Bartosh]

On Thu, Jan 05, 2017 at 10:10:50AM +0000, Richard Purdie wrote:
> On Thu, 2017-01-05 at 11:40 +0200, Ed Bartosh wrote:
> > On Wed, Jan 04, 2017 at 11:48:53PM +0000, Richard Purdie wrote:
> > > 
> > > Its possible something (like bitbake/tinfoil2) may mess around with
> > > the
> > > environment and using the enviroment as a global variable store
> > > isn't
> > > particularly nice anyway.
> > > 
> > > This patch changes the BUILDDIR usages so that the environment
> > > isn't used
> > > as a global store and a global variable is used instead. Whilst
> > > that
> > > is still not perfect, it does avoid the current double and triple
> > > backtraces
> > > we're seeing where tinfoil2/bitbake has trampled the enviroment
> > > leading
> > > to failures of failures making debugging even harder.
> > > 
> > > Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
> > > ---
> > >  scripts/oe-selftest | 19 +++++++++++++------
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/scripts/oe-selftest b/scripts/oe-selftest
> > > index bfcea66..e166521 100755
> > > --- a/scripts/oe-selftest
> > > +++ b/scripts/oe-selftest
> > > @@ -111,9 +111,13 @@ def get_args_parser():
> > >                          help='Submit test results to a
> > > repository')
> > >      return parser
> > >  
> > > +builddir = None
> > > +
> > >  
> > >  def preflight_check():
> > >  
> > > +    global builddir
> > > +
> > >      log.info("Checking that everything is in order before running
> > > the tests")
> > >  
> > >      if not os.environ.get("BUILDDIR"):
> > > @@ -135,7 +139,7 @@ def preflight_check():
> > >      return True
> > >  
> > >  def add_include():
> > > -    builddir = os.environ.get("BUILDDIR")
> > > +    global builddir
> > You don't need to use 'global' here. It's only mandatory if
> > you change variable value:
> > https://docs.python.org/3/reference/simple_stmts.html#the-global-stat
> > ement
> 
> In this case I did it purely for readability to make it clear how we
> were expecting it to work and to match the other uses. It doesn't hurt
> anything afaik.
> 
> > Would it be more readable to use name in upper case: BUILDDIR?
> 
> Not sure, that isn't something we've used as a style anywhere else so
> it doesn't really match any other style. Is that a python convention?
No, it's not. I thought it would be more readable as uppercase name
is the same as environment variable name. And it would be different
from local variable names, so you wouldn't need to use 'global'.

--
Regards,
Ed