[Piglit] [PATCH 03/13] core: replace Environment.collectData()
Dylan Baker
baker.dylan.c at gmail.com
Sun Jun 22 16:00:23 PDT 2014
On Sunday, June 22, 2014 06:54:17 PM Ilia Mirkin wrote:
> 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.
>
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
> >> > 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/ba14803e/attachment.sig>
More information about the Piglit
mailing list