[2/2] framework: Don't try to parse GL/GLSL version from wflinfo.

Submitted by Jose Fonseca on Dec. 7, 2015, 2:26 p.m.

Details

Message ID 1449498360-13792-2-git-send-email-jfonseca@vmware.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Piglit

Not browsing as part of any series.

Commit Message

Jose Fonseca Dec. 7, 2015, 2:26 p.m.
It's making many assumptions about the wflinfo which are not true.

So completely disable it as a workaround.

Though I wonder if there's really any merit in adding a depending on
wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
should have its own internally utility program to dump every thing needed.
Parsing output from external utilities is begging for trouble.
---
 framework/test/opengl.py | 5 +++++
 1 file changed, 5 insertions(+)

Patch hide | download patch | download mbox

diff --git a/framework/test/opengl.py b/framework/test/opengl.py
index 29da2d1..0a7b2d7 100644
--- a/framework/test/opengl.py
+++ b/framework/test/opengl.py
@@ -80,6 +80,11 @@  class WflInfo(object):
         opts -- arguments to pass to wflinfo other than verbose and platform
 
         """
+
+        # FIXME: The version parsing below is full of bugs.  Disable all
+        # wflinfo invocation until they are addressed
+        raise StopWflinfo('OSError')
+
         with open(os.devnull, 'w') as d:
             try:
                 raw = subprocess.check_output(

Comments

There's a bin/glinfo that it should be using. Relying on
glxinfo/wflinfo/etc seems totally inappropriate.

On Mon, Dec 7, 2015 at 9:26 AM, Jose Fonseca <jfonseca@vmware.com> wrote:
> It's making many assumptions about the wflinfo which are not true.
>
> So completely disable it as a workaround.
>
> Though I wonder if there's really any merit in adding a depending on
> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
> should have its own internally utility program to dump every thing needed.
> Parsing output from external utilities is begging for trouble.
> ---
>  framework/test/opengl.py | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> index 29da2d1..0a7b2d7 100644
> --- a/framework/test/opengl.py
> +++ b/framework/test/opengl.py
> @@ -80,6 +80,11 @@ class WflInfo(object):
>          opts -- arguments to pass to wflinfo other than verbose and platform
>
>          """
> +
> +        # FIXME: The version parsing below is full of bugs.  Disable all
> +        # wflinfo invocation until they are addressed
> +        raise StopWflinfo('OSError')
> +
>          with open(os.devnull, 'w') as d:
>              try:
>                  raw = subprocess.check_output(
> --
> 2.5.0
>
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
> It's making many assumptions about the wflinfo which are not true.
> 
> So completely disable it as a workaround.
> 
> Though I wonder if there's really any merit in adding a depending on
> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
> should have its own internally utility program to dump every thing needed.
> Parsing output from external utilities is begging for trouble.

If you don't have wflinfo, it'll go on and run anyway. It's also
designed to be as conservative as possible with versions, and we should
absolutely try/except the float if a driver doesn't return a sane
version.

Out of curiosity what driver are you using, I tested this extensively
against both i965 and against RadionSI. I had no issues at all, when
anything failed it would simply fall back to running the test and
letting the binary skip itself, as it was designed to do.

FYI:
I asked Chad (who maintains waffle) before I started this, and he made
the assertion that wflinfo output was designed for parsing by external
utilities and was considered API stable.

Also, I would prefer to have some kind of bindings for wflinfo rather
than parsing the string output, the goal was to have JSON interface
(google also wanted this interface for other uses), but neither they nor
I have had time to send anything.

Dylan

> ---
>  framework/test/opengl.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> index 29da2d1..0a7b2d7 100644
> --- a/framework/test/opengl.py
> +++ b/framework/test/opengl.py
> @@ -80,6 +80,11 @@ class WflInfo(object):
>          opts -- arguments to pass to wflinfo other than verbose and platform
>  
>          """
> +
> +        # FIXME: The version parsing below is full of bugs.  Disable all
> +        # wflinfo invocation until they are addressed
> +        raise StopWflinfo('OSError')
> +
>          with open(os.devnull, 'w') as d:
>              try:
>                  raw = subprocess.check_output(
> -- 
> 2.5.0
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
On Mon, Dec 07, 2015 at 09:32:18AM -0500, Ilia Mirkin wrote:
> There's a bin/glinfo that it should be using. Relying on
> glxinfo/wflinfo/etc seems totally inappropriate.

bin/glinfo is woefully insufficient, it doesn't do gles, It doesn't do
core profile. It doesn't know about GLX, WGL, EGL, etc. and this feature
needs that. Writing a tool to do what there are already multiple tools
do feels very Not Invented Here to me.

> 
> On Mon, Dec 7, 2015 at 9:26 AM, Jose Fonseca <jfonseca@vmware.com> wrote:
> > It's making many assumptions about the wflinfo which are not true.
> >
> > So completely disable it as a workaround.
> >
> > Though I wonder if there's really any merit in adding a depending on
> > wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
> > should have its own internally utility program to dump every thing needed.
> > Parsing output from external utilities is begging for trouble.
> > ---
> >  framework/test/opengl.py | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> > index 29da2d1..0a7b2d7 100644
> > --- a/framework/test/opengl.py
> > +++ b/framework/test/opengl.py
> > @@ -80,6 +80,11 @@ class WflInfo(object):
> >          opts -- arguments to pass to wflinfo other than verbose and platform
> >
> >          """
> > +
> > +        # FIXME: The version parsing below is full of bugs.  Disable all
> > +        # wflinfo invocation until they are addressed
> > +        raise StopWflinfo('OSError')
> > +
> >          with open(os.devnull, 'w') as d:
> >              try:
> >                  raw = subprocess.check_output(
> > --
> > 2.5.0
> >
> > _______________________________________________
> > Piglit mailing list
> > Piglit@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/piglit
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
On Mon, Dec 7, 2015 at 1:38 PM, Dylan Baker <baker.dylan.c@gmail.com> wrote:
> On Mon, Dec 07, 2015 at 09:32:18AM -0500, Ilia Mirkin wrote:
>> There's a bin/glinfo that it should be using. Relying on
>> glxinfo/wflinfo/etc seems totally inappropriate.
>
> bin/glinfo is woefully insufficient, it doesn't do gles, It doesn't do
> core profile. It doesn't know about GLX, WGL, EGL, etc. and this feature
> needs that. Writing a tool to do what there are already multiple tools
> do feels very Not Invented Here to me.

It's conceivable that wflinfo is OK to use since it uses the same
infra that piglit uses, but what bearing does glxinfo have on anything
when I'm doing a run with gbm?

  -ilia
On Mon, Dec 07, 2015 at 01:40:56PM -0500, Ilia Mirkin wrote:
> On Mon, Dec 7, 2015 at 1:38 PM, Dylan Baker <baker.dylan.c@gmail.com> wrote:
> > On Mon, Dec 07, 2015 at 09:32:18AM -0500, Ilia Mirkin wrote:
> >> There's a bin/glinfo that it should be using. Relying on
> >> glxinfo/wflinfo/etc seems totally inappropriate.
> >
> > bin/glinfo is woefully insufficient, it doesn't do gles, It doesn't do
> > core profile. It doesn't know about GLX, WGL, EGL, etc. and this feature
> > needs that. Writing a tool to do what there are already multiple tools
> > do feels very Not Invented Here to me.
> 
> It's conceivable that wflinfo is OK to use since it uses the same
> infra that piglit uses, but what bearing does glxinfo have on anything
> when I'm doing a run with gbm?
> 
>   -ilia

Nothing, and that's why the features uses waffle. And for us to
implement a tool that does what wflinfo does, would be to basically
reimplement wflinfo in piglit. That seems like a huge amount of
duplicated effort.

However, there are clearly some cases of drivers (or waffle + drivers)
being stupid that I didn't anticipate. I tested the two drivers I had
available to me (which both happen to be mesa based), so maybe Nvidia or
AMD have bugs in their proprietary drivers, or maybe the spec allows
version numbers that I didn't see when I glanced through looking to see
what was allowed.

Dylan
On 07/12/15 18:12, Dylan Baker wrote:
> On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
>> It's making many assumptions about the wflinfo which are not true.
>>
>> So completely disable it as a workaround.
>>
>> Though I wonder if there's really any merit in adding a depending on
>> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
>> should have its own internally utility program to dump every thing needed.
>> Parsing output from external utilities is begging for trouble.
>
> If you don't have wflinfo, it'll go on and run anyway.

Yes, that was the workaround I took for my test slaves.

 > It's also
> designed to be as conservative as possible with versions, and we should
> absolutely try/except the float if a driver doesn't return a sane
> version.
 >
> Out of curiosity what driver are you using, I tested this extensively
> against both i965 and against RadionSI. I had no issues at all, when
> anything failed it would simply fall back to running the test and
> letting the binary skip itself, as it was designed to do.

It happened with all drivers I normally:


- On NVIDIA it crashes because version string is "4.4.0 ...", i.e., 
"major.minor.patch".  I believe this is fine.

   Traceback (most recent call last):
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/base.py", line 
181, in execute
     self.run()
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/base.py", line 
234, in run
     self.is_skip()
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/opengl.py", line 
314, in is_skip
     if (self.__info.gl_version is not None
   File "/home/jfonseca/work/vmware/tests/piglit/framework/core.py", 
line 204, in __get__
     value = self.__func(instance)
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/opengl.py", line 
182, in gl_version
     raw.split('\n'), 'OpenGL version string').split()[3])
ValueError: invalid literal for float(): 4.4.0


   $ wflinfo --platform glx --api gl --profile core
   Waffle platform: glx
   Waffle api: gl
   OpenGL vendor string: NVIDIA Corporation
   OpenGL renderer string: Quadro K1000M/PCIe/SSE2
   OpenGL version string: 4.4.0 NVIDIA 355.11
   OpenGL context flags: 0x0


- With Mesa Xlib libGL.so.1:

Traceback (most recent call last):
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/base.py", line 
181, in execute
     self.run()
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/base.py", line 
234, in run
     self.is_skip()
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/opengl.py", line 
322, in is_skip
     if (self.__info.gles_version is not None
   File "/home/jfonseca/work/vmware/tests/piglit/framework/core.py", 
line 204, in __get__
     value = self.__func(instance)
   File 
"/home/jfonseca/work/vmware/tests/piglit/framework/test/opengl.py", line 
214, in gles_version
     raw.split('\n'), 'OpenGL version string').split()[5])

   $ wflinfo --platform glx --api gles3
   Waffle platform: glx
   Waffle api: gles3
   OpenGL vendor string: WFLINFO_GL_ERROR
   OpenGL renderer string: WFLINFO_GL_ERROR
   OpenGL version string: WFLINFO_GL_ERROR

   Not sure why this happens.  Anyway, my interest is GL + GLX, not so 
much GLES.

> FYI:
> I asked Chad (who maintains waffle) before I started this, and he made
> the assertion that wflinfo output was designed for parsing by external
> utilities and was considered API stable.
>
> Also, I would prefer to have some kind of bindings for wflinfo rather
> than parsing the string output, the goal was to have JSON interface
> (google also wanted this interface for other uses), but neither they nor
> I have had time to send anything.

All our tests use Waffle/Glut/etc.  I think we could have simple "dummy" 
tests that dump the desired versions.  I don't see the need for external 
tools or complex bindings.  By doing something different than our tests 
do, we're risking creating contexts slightly differently, and end up 
making wrong assumptions.

Jose
On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
> It's making many assumptions about the wflinfo which are not true.
> 
> So completely disable it as a workaround.
> 
> Though I wonder if there's really any merit in adding a depending on
> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
> should have its own internally utility program to dump every thing needed.
> Parsing output from external utilities is begging for trouble.
> ---
>  framework/test/opengl.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> index 29da2d1..0a7b2d7 100644
> --- a/framework/test/opengl.py
> +++ b/framework/test/opengl.py
> @@ -80,6 +80,11 @@ class WflInfo(object):
>          opts -- arguments to pass to wflinfo other than verbose and platform
>  
>          """
> +
> +        # FIXME: The version parsing below is full of bugs.  Disable all
> +        # wflinfo invocation until they are addressed
> +        raise StopWflinfo('OSError')
> +
>          with open(os.devnull, 'w') as d:
>              try:
>                  raw = subprocess.check_output(
> -- 
> 2.5.0
> 
> _______________________________________________
> Piglit mailing list
> Piglit@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit

There is bug in wflinfo about this. Wflinfo doesn't handle the optional
patch level at all, and returns an error when it is encountered.

I've opened a bug against this:
https://github.com/waffle-gl/waffle/issues/36
On 8 December 2015 at 00:40, Dylan Baker <baker.dylan.c@gmail.com> wrote:
> On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
>> It's making many assumptions about the wflinfo which are not true.
>>
>> So completely disable it as a workaround.
>>
>> Though I wonder if there's really any merit in adding a depending on
>> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
>> should have its own internally utility program to dump every thing needed.
>> Parsing output from external utilities is begging for trouble.
>> ---
>>  framework/test/opengl.py | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
>> index 29da2d1..0a7b2d7 100644
>> --- a/framework/test/opengl.py
>> +++ b/framework/test/opengl.py
>> @@ -80,6 +80,11 @@ class WflInfo(object):
>>          opts -- arguments to pass to wflinfo other than verbose and platform
>>
>>          """
>> +
>> +        # FIXME: The version parsing below is full of bugs.  Disable all
>> +        # wflinfo invocation until they are addressed
>> +        raise StopWflinfo('OSError')
>> +
>>          with open(os.devnull, 'w') as d:
>>              try:
>>                  raw = subprocess.check_output(
>> --
>> 2.5.0
>>
>> _______________________________________________
>> Piglit mailing list
>> Piglit@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/piglit
>
> There is bug in wflinfo about this. Wflinfo doesn't handle the optional
> patch level at all, and returns an error when it is encountered.
>
> I've opened a bug against this:
> https://github.com/waffle-gl/waffle/issues/36
>
Afaics wflinfo simply ignores the patch version and returns an error
on glGetError() != NO_ERROR or when the version string is NULL. In the
other direction (context creation) one cannot specify a patch version
so waffle has nothing to do in that regard.

Am I missing something ?

Cheers
Emil
On 08/12/15 00:40, Dylan Baker wrote:
> On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
>> It's making many assumptions about the wflinfo which are not true.
>>
>> So completely disable it as a workaround.
>>
>> Though I wonder if there's really any merit in adding a depending on
>> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
>> should have its own internally utility program to dump every thing needed.
>> Parsing output from external utilities is begging for trouble.
>> ---
>>   framework/test/opengl.py | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
>> index 29da2d1..0a7b2d7 100644
>> --- a/framework/test/opengl.py
>> +++ b/framework/test/opengl.py
>> @@ -80,6 +80,11 @@ class WflInfo(object):
>>           opts -- arguments to pass to wflinfo other than verbose and platform
>>
>>           """
>> +
>> +        # FIXME: The version parsing below is full of bugs.  Disable all
>> +        # wflinfo invocation until they are addressed
>> +        raise StopWflinfo('OSError')
>> +
>>           with open(os.devnull, 'w') as d:
>>               try:
>>                   raw = subprocess.check_output(
>> --
>> 2.5.0
>>
>> _______________________________________________
>> Piglit mailing list
>> Piglit@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/piglit
>
> There is bug in wflinfo about this. Wflinfo doesn't handle the optional
> patch level at all, and returns an error when it is encountered.
>
> I've opened a bug against this:
> https://github.com/waffle-gl/waffle/issues/36

FWIW, this is very different from what I saw.

As I mentioned on another email on this thread, what I see is:

   $ wflinfo --platform glx --api gl --profile core
   Waffle platform: glx
   Waffle api: gl
   OpenGL vendor string: NVIDIA Corporation
   OpenGL renderer string: Quadro K1000M/PCIe/SSE2
   OpenGL version string: 4.4.0 NVIDIA 355.11
   OpenGL context flags: 0x0

That is, Ubuntu 15.10's wflinfo is dumping the "OpenGL version string" 
*as-is*, ie, without any processing.

I don't know what was expected out of wflinfo, but I still think it's a 
mistake to rely on it.  Especially now the cat is out of the bag.


Jose
On Tue, Dec 08, 2015 at 12:25:52PM +0000, Emil Velikov wrote:
> On 8 December 2015 at 00:40, Dylan Baker <baker.dylan.c@gmail.com> wrote:
> > On Mon, Dec 07, 2015 at 02:26:00PM +0000, Jose Fonseca wrote:
> >> It's making many assumptions about the wflinfo which are not true.
> >>
> >> So completely disable it as a workaround.
> >>
> >> Though I wonder if there's really any merit in adding a depending on
> >> wflinfo.  IMO, if piglit cares for the advertised GL/GLSL versions, it
> >> should have its own internally utility program to dump every thing needed.
> >> Parsing output from external utilities is begging for trouble.
> >> ---
> >>  framework/test/opengl.py | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/framework/test/opengl.py b/framework/test/opengl.py
> >> index 29da2d1..0a7b2d7 100644
> >> --- a/framework/test/opengl.py
> >> +++ b/framework/test/opengl.py
> >> @@ -80,6 +80,11 @@ class WflInfo(object):
> >>          opts -- arguments to pass to wflinfo other than verbose and platform
> >>
> >>          """
> >> +
> >> +        # FIXME: The version parsing below is full of bugs.  Disable all
> >> +        # wflinfo invocation until they are addressed
> >> +        raise StopWflinfo('OSError')
> >> +
> >>          with open(os.devnull, 'w') as d:
> >>              try:
> >>                  raw = subprocess.check_output(
> >> --
> >> 2.5.0
> >>
> >> _______________________________________________
> >> Piglit mailing list
> >> Piglit@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/piglit
> >
> > There is bug in wflinfo about this. Wflinfo doesn't handle the optional
> > patch level at all, and returns an error when it is encountered.
> >
> > I've opened a bug against this:
> > https://github.com/waffle-gl/waffle/issues/36
> >
> Afaics wflinfo simply ignores the patch version and returns an error
> on glGetError() != NO_ERROR or when the version string is NULL. In the
> other direction (context creation) one cannot specify a patch version
> so waffle has nothing to do in that regard.
> 
> Am I missing something ?
> 
> Cheers
> Emil

No, I misread the code. I have closed the bug. Sorry for the noise.
On 7 December 2015 at 22:43, Jose Fonseca <jfonseca@vmware.com> wrote:
...
> - With Mesa Xlib libGL.so.1:
>
...
>
>   $ wflinfo --platform glx --api gles3
>   Waffle platform: glx
>   Waffle api: gles3
>   OpenGL vendor string: WFLINFO_GL_ERROR
>   OpenGL renderer string: WFLINFO_GL_ERROR
>   OpenGL version string: WFLINFO_GL_ERROR
>
>   Not sure why this happens.  Anyway, my interest is GL + GLX, not so much
> GLES.
>
Afaict this happens as we try to mix Xlib based libGL with libGLESx.
That sort of thing isn't supported in mesa, and we either 1) need to
tweak waffle to honour GLX_EXT_create_context_es*_profile or 2) have a
bug in Xlib libGL, where it reports
GLX_EXT_create_context_es*_profile.

Can you confirm/dismiss the latter and I'll take a look on the former.
If you can throw in a github bugreport that'll be great.

Thanks
Emil