[Piglit] [PATCH 03/13] core: replace Environment.collectData()
Ilia Mirkin
imirkin at alum.mit.edu
Sun Jun 22 15:54:17 PDT 2014
On Sun, Jun 22, 2014 at 6:46 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> 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
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.
>
>>
>> > +
>> > + 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.
>
>>
>> > +
>> > + 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
More information about the Piglit
mailing list