[Piglit] [PATCH 03/13] core: replace Environment.collectData()

[Ilia Mirkin]

On Sat, Jun 21, 2014 at 8:06 AM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> Environment.collectData() is strange and awful for a host of reasons.
> 1) Its implementation was such that it should have been a static method,
>    but it wasn't
> 2) It wasn't at all related to Environment
> 3) It used a public helper which it was the sole consumer of.
>
> Replacing it with a top level function makes a lot more sense for all of
> these reason. This function has another major advantage, it assumes
> nothing. In the former implementation it was assumed that specific
> operating systems had specific tools available, and that those tools
> were only available on that platform. This implementation doesn't assume
> that, instead it makes a blind leap that the tools is available, and
> then is caught if its wrong.
>
> Signed-off-by: Dylan Baker <baker.dylan.c at gmail.com>
> ---
>  framework/core.py         | 49 ++++++++++++++++++++++++++---------------------
>  framework/programs/run.py |  4 ++--
>  2 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/framework/core.py b/framework/core.py
> index 84832cf..f61548d 100644
> --- a/framework/core.py
> +++ b/framework/core.py
> @@ -25,7 +25,6 @@
>  from __future__ import print_function
>  import errno
>  import os
> -import platform
>  import re
>  import subprocess
>  import sys
> @@ -46,6 +45,7 @@ __all__ = ['PIGLIT_CONFIG',
>             'TestResult',
>             'JSONWriter',
>             'checkDir',
> +           'collect_system_info',
>             'load_results',
>             'parse_listfile']
>
> @@ -368,28 +368,33 @@ class Environment:
>              else:
>                  yield (key, values)
>
> -    def run(self, command):
> +
> +def collect_system_info():
> +    """ Get relavent information about the system running piglit
> +
> +    This method runs through a list of tuples, where element 1 is the name of
> +    the program being run, and elemnt 2 is a command to run (in a form accepted
> +    by subprocess.Popen)
> +
> +    """
> +    progs = [('wglinfo', ['wglinfo']),
> +             ('glxinfo', ['glxinfo']),
> +             ('uname', ['uname', '-a']),
> +             ('lspci', ['lspci'])]

Not a specific issue with this change, but e.g. on my machine lspci is
in /usr/sbin (which is, of course, not in $PATH since I'm not root).
Don't know if it's worthwhile to check /sbin, /usr/sbin,
/usr/local/sbin for it.

> +
> +    result = {}
> +
> +    for name, command in progs:
>          try:
> -            p = subprocess.Popen(command,
> -                                 stdout=subprocess.PIPE,
> -                                 stderr=subprocess.PIPE,
> -                                 universal_newlines=True)
> -            (stdout, stderr) = p.communicate()
> -        except:
> -            return "Failed to run " + command
> -        return stderr+stdout
> -
> -    def collectData(self):
> -        result = {}
> -        system = platform.system()
> -        if (system == 'Windows' or system.find("CYGWIN_NT") == 0):
> -            result['wglinfo'] = self.run('wglinfo')
> -        else:
> -            result['glxinfo'] = self.run('glxinfo')
> -        if system == 'Linux':
> -            result['uname'] = self.run(['uname', '-a'])
> -            result['lspci'] = self.run('lspci')
> -        return result
> +            result[name] = subprocess.check_output(command,
> +                                                   stderr=subprocess.STDOUT)
> +        except OSError as e:
> +            # If we get the 'no file or directory' error then pass, that means
> +            # that the binary isn't installed or isn't relavent to the system
> +            if e.errno != 2:
> +                raise

Is a failure here important enough to tank the whole piglit run? It
didn't before...

> +
> +    return result
>
>
>  def load_results(filename):
> diff --git a/framework/programs/run.py b/framework/programs/run.py
> index b99d884..0189e48 100644
> --- a/framework/programs/run.py
> +++ b/framework/programs/run.py
> @@ -174,7 +174,7 @@ def run(input_):
>
>      json_writer.write_dict_item('name', results.name)
>
> -    for (key, value) in env.collectData().items():
> +    for key, value in core.collect_system_info().iteritems():
>          json_writer.write_dict_item(key, value)
>
>      profile = framework.profile.merge_test_profiles(args.test_profile)
> @@ -232,7 +232,7 @@ def resume(input_):
>      json_writer.close_dict()
>
>      json_writer.write_dict_item('name', results.name)
> -    for (key, value) in env.collectData().items():
> +    for key, value in core.collect_system_info().iteritems():
>          json_writer.write_dict_item(key, value)
>
>      json_writer.write_dict_key('tests')
> --
> 2.0.0
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit