Re: [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@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 <jfons...@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

_______________________________________________
Piglit mailing list
Piglit@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/piglit