[Piglit] [PATCH 31/35] arb_gpu_shader5: Add support for testing spirv with XFB streams

[Alejandro Piñeiro]

On 20/08/18 04:11, Timothy Arceri wrote:
> On 18/08/18 19:30, Alejandro Piñeiro wrote:
>> On 18/08/18 06:36, Timothy Arceri wrote:
>>> On 18/08/18 14:32, Timothy Arceri wrote:
>>>> Won't this cause shader runner to needlessly parse the .shader_test
>>>> file?
>>
>> True, good point. In any case, as the test lacks a [test] section, full
>> shader.py runs would just check if the test links (assuming that
>> ARB_gl_spirv are available). Having said so ...
>>
>>>
>>> The file extension is also confusing. Maybe we should name these type
>>> of files .shader_source or something similar rather than
>>> .shader_test ???
>>
>> ... this makes sense. That name fits better, and would avoid the
>> previous issue. As that would be a small change, I will make the change
>> locally, while I wait for the review of the other patches.
>
> I've skimmed over most of the series and I didn't see anything other
> than this that I had issue with. It's not very common to get a full
> review for a piglit series. The general rule is once its been on the
> list for a few weeks it's usually ok to push. After all it's better to
> have wrong tests that can be fixed later than to have no tests at all.
>
> I understand the external dependency might have stopped you from just
> pushing but since its an optional dependency it's fine IMO.

Yes, exactly, that was the only reason I didn't push it yet. FWIW, I was
expecting it to be somewhat controversial. But I always tend to be on
the pessimistic/conservative side of things.

>
> Anyway for the series feel free to add:
>
> Acked-by: Timothy Arceri <tarceri at itsqueeze.com>

Ok, thank you very much.

As the series was around several weeks, I think that I will push it as
it is. If the optional dependency causes some issues, at least pushing
it would get the attention of people.

BR