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

[Brian Paul]

On 06/02/2014 11:14 AM, Jose Fonseca wrote:
>
>
> ----- 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.

Yeah, there's more unfinished business with 'testsDir' to tend to.  I've 
only dug into the obviously broken cases I care about right now.  The 
code's a bit complicated and I don't have the time right now to make any 
big changes.

-Brian