[Piglit] [PATCH 3/3] all.py: use os.path.join() and string.split() to fix things for cygwin

[Jose Fonseca]

----- Original Message -----
> On Sunday, June 01, 2014 03:03:22 PM Brian Paul wrote:
> > Use os.path.join() to build up file paths instead of using string
> > concatenation.  In plain_test(), split up the arguments string
> > using string.split() instead of shlex.split() because the later
> > removes the '\' characters in Windows paths, which causes things to
> > blow up.
> > 
> > I can't find any particular reason why shlex.split() was used in
> > the first place.
> > ---
> >  tests/all.py |   17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/all.py b/tests/all.py
> > index bf174b7..a7e408c 100644
> > --- a/tests/all.py
> > +++ b/tests/all.py
> > @@ -7,8 +7,9 @@ import itertools
> >  import os
> >  import os.path as path
> >  import platform
> > -import shlex
> > +import string
> >  import sys
> > +import string
> 
> you've double imported string, but there's no reason to use string at all,
> see
> later comments
> 
> > 
> >  from framework.profile import TestProfile
> >  from framework.exectest import PiglitTest
> > @@ -33,7 +34,7 @@ generatedTestDir = path.normpath(path.join(
> > 
> >  # Quick wrapper for PiglitTest for our usual concurrent args.
> >  def plain_test(args):
> > -    return PiglitTest(shlex.split(args) + ['-auto'])
> > +    return PiglitTest(string.split(args) + ['-auto'])
> 
> python's str class has a split method already built-in, just use args.split()
> 
> It doesn't seem like we rely on the extended power of shlex.split(), and this
> patch doesn't seem break anything on my linux system
> 
> > 
> >  def add_single_param_test_set(group, name, *params):
> >      for param in params:
> > @@ -558,7 +559,8 @@ for subtest in ('interstage', 'intrastage', 'vs-gs'):
> >      shaders[cmdline] = concurrent_test(cmdline)
> > 
> >  def add_vpfpgeneric(group, name):
> > -    group[name] = concurrent_test('vpfp-generic ' + testsDir +
> > '/shaders/generic/' + name + '.vpfp') +    group[name] =
> > concurrent_test('vpfp-generic ' +
> > +        os.path.join(testsDir, 'shaders', 'generic', name + '.vpfp'))
> > 
> >  glx = {}
> >  add_msaa_visual_plain_tests(glx, 'glx-copy-sub-buffer')
> > @@ -1471,11 +1473,11 @@ add_plain_test(arb_draw_elements_base_vertex,
> > 'arb_draw_elements_base_vertex-mul arb_draw_instanced = {}
> >  spec['ARB_draw_instanced'] = arb_draw_instanced
> >  import_glsl_parser_tests(arb_draw_instanced,
> > -                        testsDir + '/spec/arb_draw_instanced',
> > +                        os.path.join(testsDir, 'spec',
> > 'arb_draw_instanced'), [''])
> > 
> >  add_shader_test_dir(arb_draw_instanced,
> > -                    testsDir + '/spec/arb_draw_instanced/execution',
> > +                    os.path.join(testsDir, 'spec', 'arb_draw_instanced',
> > 'execution'), recursive=True)
> >  arb_draw_instanced['dlist'] = concurrent_test('arb_draw_instanced-dlist')
> >  arb_draw_instanced['elements'] =
> > concurrent_test('arb_draw_instanced-elements') @@ -1622,7 +1624,7 @@
> > add_plain_test(arb_framebuffer_srgb, 'framebuffer-srgb') # must not be
> > concurren arb_gpu_shader5 = {}
> >  spec['ARB_gpu_shader5'] = arb_gpu_shader5
> >  add_shader_test_dir(arb_gpu_shader5,
> > -                    testsDir + '/spec/arb_gpu_shader5',
> > +                    os.path.join(testsDir, 'spec', 'arb_gpu_shader5'),
> >                      recursive=True)
> >  import_glsl_parser_tests(arb_gpu_shader5,
> >                           testsDir + '/spec/arb_gpu_shader5', [''])
> > @@ -3238,7 +3240,8 @@ add_plain_test(fast_color_clear,
> > 'fcc-read-to-pbo-after-clear') asmparsertest = {}
> >  def add_asmparsertest(group, shader):
> >      asmparsertest[group + '/' + shader] = concurrent_test(
> > -        'asmparsertest ' + group + ' ' + testsDir +
> > '/asmparsertest/shaders/' + group + '/' + shader) +        'asmparsertest '
> > + group + ' ' +
> > +        os.path.join(testsDir, 'asmparsertest', 'shaders', group, shader))
> > 
> >  add_asmparsertest('ARBfp1.0', 'abs-01.txt')
> >  add_asmparsertest('ARBfp1.0', 'abs-02.txt')
> 
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/piglit&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=NMr9uy2iTjWVixC0wOcYCWEIYhfo80qKwRgdodpoDzA%3D%0A&m=tL7dCuxgmgW1sLFr9VSAdXeC38pkIXVGo%2BsfsuDpaZ0%3D%0A&s=55aa8aee88f4ea2bd6c6b8e6499e4b9289d2d023f3f2b6b65698efb89eeb39b9
> 


I didn't spot any other problems with the series.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>



But BTW, I recently noticed that some tests don't prefix "testDir" to the args (see for example, the recently added glsl-link-test tests), it things still work because getenv(PIGLIT_SOURCE_DIR) is prefixed at run time.

Maybe instead of maintaining all this testsDir prefixing logic it would be easier just to remove it all together, and ensure that PIGLIT_SOURCE_DIR is always observed.


Jose