[Piglit] [PATCH 1/2] shader_runner regression with gles
Tom Gall
tom.gall at linaro.org
Mon Jan 21 12:33:46 PST 2013
On Mon, Jan 21, 2013 at 12:39 PM, Paul Berry <stereotype441 at gmail.com> wrote:
> 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.
Ok. Thanks for the feedback.
> 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?
Hmm looking at the regex again, you're right. I'd misinterpreted the
matching there, in particular where and how the \s* was being applied.
> 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).
I was entirely wrong to not draw attention to the new syntax which I
should have done as compared to focusing on the effect of the bug
which was to break GLES tests. My fault.
>> 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.
Ok. Thanks!
>>
>>
>> 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?
Again you're right due to my previously mentioned misinterpretation.
>>
>> 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
>
>
--
Regards,
Tom
"Where's the kaboom!? There was supposed to be an earth-shattering
kaboom!" Marvin Martian
Tech Lead, Graphics Working Group | Linaro.org │ Open source software
for ARM SoCs
w) tom.gall att linaro.org
h) tom_gall att mac.com
More information about the Piglit
mailing list