[Piglit] [PATCH 02/13] framework: Dont rely on os.environ so much
Dylan Baker
baker.dylan.c at gmail.com
Mon Jun 23 04:26:37 PDT 2014
On Saturday, June 21, 2014 09:26:42 AM Ilia Mirkin wrote:
> On Sat, Jun 21, 2014 at 8:06 AM, Dylan Baker <baker.dylan.c at gmail.com>
wrote:
> > There are some problems with using os.environ, all of which are caused
> > shell child-variable relationships. Most of these problems are easy to
> > solve, since we don't actually need to have environment variables set
> > except during test execution, and that can be passed to Popen (or it's
>
> its
>
> > helper wrappers). This gives us much better assurance that things are
> > happening in the way we expect.
>
> Also in the subject: "Don't"
>
> > Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> > ---
> >
> > framework/core.py | 23 ++++++++---------------
> > framework/exectest.py | 15 +++++++++++----
> > framework/programs/run.py | 21 +++++++++------------
> > piglit | 15 ++++++---------
> > 4 files changed, 34 insertions(+), 40 deletions(-)
> >
> > diff --git a/framework/core.py b/framework/core.py
> > index cd72956..84832cf 100644
> > --- a/framework/core.py
> > +++ b/framework/core.py
> >
> > @@ -206,21 +206,6 @@ def checkDir(dirname, failifexists):
> > if e.errno != errno.EEXIST:
> > raise
> >
> > -if 'PIGLIT_SOURCE_DIR' not in os.environ:
> > - p = os.path
> > - os.environ['PIGLIT_SOURCE_DIR'] =
> > p.abspath(p.join(p.dirname(__file__), -
> > '..'))
> > -
> > -# In debug builds, Mesa will by default log GL API errors to stderr.
> > -# This is useful for application developers or driver developers
> > -# trying to debug applications that should execute correctly. But for
> > -# piglit we expect to generate errors regularly as part of testing,
> > -# and for exhaustive error-generation tests (particularly some in
> > -# khronos's conformance suite), it can end up ooming your system
> > -# trying to parse the strings.
> > -if 'MESA_DEBUG' not in os.environ:
> > - os.environ['MESA_DEBUG'] = 'silent'
> > -
> >
> > class TestResult(dict):
> > def __init__(self, *args):
> > @@ -353,6 +338,14 @@ class Environment:
> > self.valgrind = valgrind
> > self.dmesg = dmesg
> > self.verbose = verbose
> >
> > + # env is used to set some base environment variables that are not
> > going + # to change across runs, without sending them to
> > os.environ which is + # fickle as easy to break
> > + self.env = {
> > + 'PIGLIT_SOURCE_DIR': os.path.abspath(
> > + os.path.join(os.path.dirname(__file__), '..')),
>
> The old logic would inherit a PIGLIT_SOURCE_DIR if one was passed in.
> Was there some sort of reason for it? (Not sure if there was, so just
> asking the question :) )
Yeah, I'm not really sure why someone would want to do that, it seems like a
quick and easy way to break piglit in strange ways.
>
> > + 'MESA_DEBUG': 'silent',
> > + }
> >
> > """
> > The filter lists that are read in should be a list of string
> > objects,
> >
> > diff --git a/framework/exectest.py b/framework/exectest.py
> > index e55274e..ad6f2ae 100644
> > --- a/framework/exectest.py
> > +++ b/framework/exectest.py
> >
> > @@ -136,7 +136,9 @@ class Test(object):
> > """
> > self.result['command'] = ' '.join(self.command)
> > self.result['environment'] = " ".join(
> >
> > - '{0}="{1}"'.format(k, v) for k, v in self.env.iteritems())
> > + '{0}="{1}"'.format(k, v) for k, v in self.env.iteritems()) +
> > \
> > + " ".join(
> > + '{0}="{1}"'.format(k, v) for k, v in
> > self.ENV.env.iteritems())
> Isn't there some "append these two iterators" function somewhere?
> itertools.chain it seems.
>
> > if self.check_for_skip_scenario():
> > self.result['result'] = 'skip'
> >
> > @@ -188,9 +190,14 @@ class Test(object):
> > return False
> >
> > def get_command_result(self):
> > - fullenv = os.environ.copy()
> > - for key, value in self.env.iteritems():
> > - fullenv[key] = str(value)
> > + # Set the environment for the tests. Use the default settings
> > created + # in the Environment constructor first, then use any
> > user defined + # variables, finally, use any variables set for the
> > test in the test + # profile
> > + fullenv = self.ENV.env.copy()
> > + for iterable in [os.environ, self.env]:
>
> > + for key, value in iterable.iteritems():
> for key, value in itertools.chain(os.environ.iteritems(),
> self.env.iteritems())
> > + fullenv[key] = str(value)
> >
> > try:
> > proc = subprocess.Popen(self.command,
> >
> > diff --git a/framework/programs/run.py b/framework/programs/run.py
> > index 16d4e1d..b99d884 100644
> > --- a/framework/programs/run.py
> > +++ b/framework/programs/run.py
> >
> > @@ -116,10 +116,6 @@ def run(input_):
> > | SEM_NOOPENFILEERRORBOX
> >
> > ctypes.windll.kernel32.SetErrorMode(uMode)
> >
> > - # Set the platform to pass to waffle
> > - if args.platform:
> > - os.environ['PIGLIT_PLATFORM'] = args.platform
> > -
> >
> > # If dmesg is requested we must have serial run, this is becasue
> > dmesg
> > # isn't reliable with threaded run
> >
> > if args.dmesg:
> > @@ -130,8 +126,8 @@ def run(input_):
> > core.PIGLIT_CONFIG.readfp(args.config_file)
> > args.config_file.close()
> >
> > else:
> > -
> > core.PIGLIT_CONFIG.read(os.path.join(os.environ['PIGLIT_SOURCE_DIR'], -
> > 'piglit.conf'))
> > + core.PIGLIT_CONFIG.read(os.path.abspath(
> > + os.path.join(os.path.dirname(__file__), 'piglit.conf')))
>
> Is that the right place to look for piglit.conf? There should probably
> be two '..''s in there... I think this whole thing might be easier to
> resolve if you add a framework/__init__.py which computes the
> top-level dir, to be reused everywhere.
how do you import from an __init__,py at the same level? I though __init__.py
defined the view of the package when importing.
>
> > # Pass arguments into Environment
> > env = core.Environment(concurrent=args.concurrency,
> >
> > @@ -142,6 +138,11 @@ def run(input_):
> > dmesg=args.dmesg,
> > verbose=args.verbose)
> >
> > + # Set the platform to pass to waffle
> > + if args.platform:
> > + env.env['PIGLIT_PLATFORM'] = args.platform
> > +
> > +
> >
> > # Change working directory to the root of the piglit directory
> > piglit_dir = path.dirname(path.realpath(sys.argv[0]))
> > os.chdir(piglit_dir)
> >
> > @@ -218,12 +219,8 @@ def resume(input_):
> > dmesg=results.options['dmesg'],
> > verbose=results.options['verbose'])
> >
> > - # attempt to restore a saved platform, if there is no saved platform
> > just - # go on
> > - try:
> > - os.environ['PIGLIT_PLATFORM'] = results.options['platform']
> > - except KeyError:
> > - pass
> > + if results.options.get('platform'):
> > + env.env['PIGLIT_PLATFORM'] = results.options['platform']
> >
> > results_path = path.join(args.results_path, "main")
> > json_writer = core.JSONWriter(open(results_path, 'w+'))
> >
> > diff --git a/piglit b/piglit
> > index 616e408..4c9a24e 100755
> > --- a/piglit
> > +++ b/piglit
> > @@ -36,25 +36,24 @@ import os.path as path
> >
> > import sys
> > import argparse
> >
> > -# Setting PIGLIT_SOURCE_DIR (and by extension sys.path) is actually
> > pretty
> > -# complicated, since there are three seperate uses we need to detect:
> > +# Setting sys.path is actually pretty complicated, since there are three
> >
> > +# seperate uses we need to detect:
> > # 1) piglit is being run in the source directory, built in tree
> > # 2) piglit is being run from the source directory outside of it, built
> > in tree # 3) piglit has been built out of tree and installed, and is
> > being run in or # out of the install directory
> >
> > +# Case one is the implicit case. In this event nothing needs to be set,
> > it
> > +# should "just work" (tm)
> > +
> >
> > # It is critical that this block be run before importing anything from
> > # framework (as there is no gaurantee that framework will be in python's
> > path # before this blck is run)
> >
> > -# Case 1
> > -if path.exists('framework/exectest.py'):
> > - os.environ['PIGLIT_SOURCE_DIR'] = path.abspath(path.curdir)
> > -else:
> >
> > +if not path.exists('framework/exectest.py'):
> > dirpath = path.dirname(path.abspath(__file__))
> > # Case 2
> >
> > if path.exists(path.join(dirpath, 'framework/exectest.py')):
> > - os.environ['PIGLIT_SOURCE_DIR'] = dirpath
> >
> > sys.path.append(dirpath)
> >
> > # Case 3
> >
> > else:
> > @@ -66,8 +65,6 @@ else:
> > # piglits installed as piglit${the_date_of_install}, and we need
> > to
> > # detect that.
> > install_path = path.abspath(path.join(dirpath, '..', 'lib',
> > piglit))
> >
> > -
> > - os.environ['PIGLIT_SOURCE_DIR'] = install_path
> >
> > sys.path.append(install_path)
> >
> > import framework.programs.run as run
> >
> > --
> > 2.0.0
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20140623/fde030ea/attachment-0001.sig>
More information about the Piglit
mailing list