On Sat, Jun 21, 2014 at 8:06 AM, Dylan Baker <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/piglit _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
