On Saturday, June 21, 2014 09:37:05 AM Ilia Mirkin wrote: > On Sat, Jun 21, 2014 at 8:06 AM, Dylan Baker <[email protected]> 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 <[email protected]> > > --- > > > > 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
> > [email protected]
> > http://lists.freedesktop.org/mailman/listinfo/piglit
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
