[Piglit] [PATCH 02/13] framework: Dont rely on os.environ so much
Ilia Mirkin
imirkin at alum.mit.edu
Sat Jun 21 06:26:42 PDT 2014
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 :) )
> + '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.
>
> # 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
More information about the Piglit
mailing list