[Piglit] [PATCH 1/2] shader_runner regression with gles

[Paul Berry]

I'd suggest changing to a commit subject that describes precisely what
change you're making in the active voice, e.g. something like "Fix
shader_runner regexps to match new version requirement syntax".  The
existing commit subject ("shader_runner regression with gles") is too vague.

On 17 January 2013 15:08, Tom Gall <tom.gall at linaro.org> wrote:

> Augment the parser that examines the shader_test file passed in
> with the test to determine which brand of shader_running to
> execute, shader_runner, shader_runner_gles2, etc.
>
> This fixes a bug where GLSL ES if first in the [required] section
> could match against GL after which the wrong shader_runner command
> would be used due to the wrong gl api being used.
>

This seems like a red herring to me.  How would "GLSL ES" have matched any
of the preexisting regular expressions?  It seems like the substantive
change you're making here is to change the regular expressions from
matching "GL >= N.M es" (which was the correct syntax before Stuart
Abercrombie's patch "shader_runner: Alter GL/GLSL ES version requirement
syntax.") to matching "GL ES >= N.M" (which is the correct syntax now).


>
> Last if plain GLSL is found, GL is presumed as the api. If GLSL ES
> is found, GL ES is presumed as the api.
>

This seems reasonable, but it's a separate feature and should be in a
separate patch.


>
> When dispatch is implemented for GLES all this can go away.
>
> Signed-off-by: Tom Gall <tom.gall at linaro.org>
>
---
>  framework/shader_test.py |   26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/framework/shader_test.py b/framework/shader_test.py
> index 335a6c1..4db291b 100755
> --- a/framework/shader_test.py
> +++ b/framework/shader_test.py
> @@ -107,16 +107,18 @@ class ShaderTest(PlainExecTest):
>          common = {
>              'cmp' : r'(<|<=|=|>=|>)',
>              'gl_version' : r'(\d.\d)',
> -            'gles2_version' : r'(2.\d\s+es)',
> -            'gles3_version' : r'(3.\d\s+es)',
> +            'gles2_version' : r'(2.\d\s)',
> +            'gles3_version' : r'(3.\d\s)',
>              'comment' : r'(#.*)',
>          }
>
>          cls.__re_require_header =
> re.compile(r'^\s*\[require\]\s*{comment}?$'.format(**common))
>          cls.__re_end_require_block = re.compile(r'^\s*\['.format(*common))
>          cls.__re_gl =
> re.compile(r'^\s*GL\s*{cmp}\s*{gl_version}\s*{comment}?$'.format(**common))
> -        cls.__re_gles2 =
> re.compile(r'^\s*GL\s*{cmp}\s*{gles2_version}\s*{comment}?$'.format(**common))
> -        cls.__re_gles3 =
> re.compile(r'^\s*GL\s*{cmp}\s*{gles3_version}\s*{comment}?$'.format(**common))
> +        cls.__re_gles2 = re.compile(r'^\s*GL
> ES\s*{cmp}\s*{gles2_version}\s*{comment}?$'.format(**common))
> +        cls.__re_gles3 = re.compile(r'^\s*GL
> ES\s*{cmp}\s*{gles3_version}\s*{comment}?$'.format(**common))
> +        cls.__re_glsl =
> re.compile(r'^\s*GLSL\s*{cmp}\s*{gl_version}\s*{comment}?$'.format(**common))
> +        cls.__re_glsles = re.compile(r'^\s*GLSL
> ES\s*{cmp}\s*{gl_version}\s*{comment}?$'.format(**common))
>          cls.__re_gl_unknown =
> re.compile(r'^\s*GL\s*{cmp}'.format(**common))
>
>      def __init__(self, shader_runner_args, run_standalone=False):
> @@ -174,15 +176,23 @@ class ShaderTest(PlainExecTest):
>                          else:
>                              continue
>                      elif parse_state == PARSE_FIND_GL_REQUIREMENT:
> -                        if cls.__re_gl.match(line) is not None:
> -                            self.__gl_api = ShaderTest.API_GL
> -                            return
> -                        elif cls.__re_gles2.match(line) is not None:
> +                        if cls.__re_gles2.match(line) is not None:
>

I don't think it should be necessary to remove the cls.__re_gl test from
here and put it at the end.  The regular expression for cls.__re_gl used to
overlap with those for cls.__re_gles2 and cls.__re_gles3, but thanks to the
other changes in your patch, it doesn't anymore, so order should be
irrelevant.  Or am I missing something?


>                              self.__gl_api = ShaderTest.API_GLES2
>                              return
>                          elif cls.__re_gles3.match(line) is not None:
>                              self.__gl_api = ShaderTest.API_GLES3
>                              return
> +                        elif cls.__re_glsles.match(line) is not None:
> +                                                       if self.__gl_api
> is None:
> +
> self.__gl_api = ShaderTest.API_GLES2
> +                                                       return
> +                        elif cls.__re_glsl.match(line) is not None:
> +                                                       if self.__gl_api
> is None:
> +
> self.__gl_api = ShaderTest.API_GL
> +                                                       return
> +                        elif cls.__re_gl.match(line) is not None:
> +                            self.__gl_api = ShaderTest.API_GL
> +                            return
>                          elif cls.__re_gl_unknown.match(line) is not None:
>                              self.__report_failure("Failed to parse GL
> requirement: " + line)
>                              self.__gl_api = ShaderTest.API_ERROR
> --
> 1.7.10.4
>
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130121/26f369ab/attachment.html>