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

[Dylan Baker]

On Saturday, June 21, 2014 09:37:05 AM Ilia Mirkin wrote:
> 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.

eh, I think you're pretty unique in not having /sbin and /usr/sbin in your 
path. I mean, I like to be able to use dd, ifconfig, lspci, uname, and a host 
of other useful programs without guessing which folder my distro put them in. 
It also makes tab completion with sudo work

> 
> > +
> > +    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...

What other other errors could we hit there? We can drop the raise, but it 
feels to me like anything other than a 'no file' error is pretty serious.

> 
> > +
> > +    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
-------------- 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/20140622/58c5280c/attachment.sig>