On Sunday, June 22, 2014 06:54:17 PM Ilia Mirkin wrote: > On Sun, Jun 22, 2014 at 6:46 PM, Dylan Baker <[email protected]> wrote: > > 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 > > Perhaps. Well, I'm gentoo, and pretty sure I never did anything > special to not have it my PATH. I've never seen /sbin & co in a user's > PATH, but I tend to be old-school in those regards. (I also don't use > sudo, so the tab completion thing never came up. And when I have used > sudo, it's for 'sudo bash'...) It's not a big deal either way... this > is not a vital component of piglit. >
Gentoo has been pretty sane to me in this regard, lspci is the only useful
thing I found in *sbin. on Debian however, it was a completely different
story.
> >> > +
> >> > + 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.
>
> OK. I was mainly pointing out that it was a behavioural change from
> the earlier logic.
Yeah well, piglit and it's bare excepts...
>
> >> > +
> >> > + 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
