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 <tarc...@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 _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/piglit
