Bug 134070 – [GTK] Load the llvmpipe (Mesa) libraries when running the tests with Xvfb

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

On r170040 <http://trac.webkit.org/changeset/170040> the Mesa GL libraries where added as also a mechanism to load them when running the tests with the Xvfb driver.

However, it seems that the mechanism to load them is still not working as expected.

Also I discovered that the Xvfb that is run is the system one, the Xvfb we ship inside the jhbuild is not used :\

I tested to run the full layout tests on my Nvidia machine and the patch didn't fixed the issue with non-mesa (aka propietary) drivers. Unless I run the tests with the workaround documented at https://trac.webkit.org/wiki/WebKitGtkLayoutTests#PropietarygraphicsdriversNVIDIAAMDproblems I still get *tons* of crashes and timeouts and many errors complaining about extension "GLX" missing on display. 

For example:


$ Tools/Scripts/run-webkit-tests --debug-rwt-logging --release --webkit-test-runner --gtk  animations/animation-iteration-event-destroy-renderer.html
[...]
21:37:44.774 5311 WebProcess crash, pid = None, error_line = #CRASHED - WebProcess
21:37:44.774 5311 killed pid 5351
21:37:44.775 5311 worker/0 animations/animation-iteration-event-destroy-renderer.html crashed, (stderr lines):
21:37:44.775 5311   Xlib:  extension "GLX" missing on display ":1".
21:37:44.776 5311   Xlib:  extension "GLX" missing on display ":1".
21:37:44.776 5311   libEGL warning: GLX/DRI2 is not supported
21:37:44.776 5311   Xlib:  extension "GLX" missing on display ":1".
21:37:44.776 5311   Xlib:  extension "GLX" missing on display ":1".
21:37:44.776 5311   Xlib:  extension "GLX" missing on display ":1".
21:37:44.777 5311 [1/1] animations/animation-iteration-event-destroy-renderer.html failed unexpectedly (WebProcess crashed)
[...]


I have been working on a patch to fix this, so I will upload it for review.

[Carlos Alberto Lopez Perez]

Created attachment 233376 [details]
Patch

[Martin Robinson]

Comment on attachment 233376 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233376&action=review

> Tools/Scripts/webkitpy/port/xvfbdriver.py:48
> +        if os.path.exists(port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> +                xvfb_found = port.host.executive.run_command(port._jhbuild_wrapper + ['which', 'Xvfb'], return_exit_code=True) is 0
> +        else:
> +                xvfb_found = port.host.executive.run_command(['which', 'Xvfb'], return_exit_code=True) is 0

Wouldn't it be easier to simply run Xvfb with the JHBuild wrapper? Doesn't the PATH take care of choosing the correct Xvfb?

> Tools/Scripts/webkitpy/port/xvfbdriver.py:58
> +        if self._port.name().startswith("gtk") and os.path.exists(self._port.path_from_webkit_base('WebKitBuild', 'Dependencies')):

I'd really like to see this move to the port specific file somehow.

> Tools/Scripts/webkitpy/port/xvfbdriver.py:61
> +                        error_handler=self._port.host.executive.ignore_error).strip()

Nit: Missing some spaces around the = sign.

> Tools/Scripts/webkitpy/port/xvfbdriver.py:99
> +        if os.path.exists(self._port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> +                # Run the Xvfb from the jhbuild
> +                run_xvfb = self._port._jhbuild_wrapper + run_xvfb

We don't have any helpers for this?

> Tools/Scripts/webkitpy/port/xvfbdriver.py:107
> +        if self._llvmpipe_libgl_path:
> +                # Force the Gallium llvmpipe software rasterizer
> +                environment['LD_LIBRARY_PATH'] = self._llvmpipe_libgl_path
> +                # If there were another paths on LD_LIBRARY_PATH append them after llvmpipe_libgl_path
> +                if os.environ.get('LD_LIBRARY_PATH'):
> +                        environment['LD_LIBRARY_PATH'] += ':%s' % os.environ.get('LD_LIBRARY_PATH')
> +

Can't we set this up in gtk.py? Your indentation is also a bit funky.

[Carlos Alberto Lopez Perez]

(In reply to comment #2)
> (From update of attachment 233376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233376&action=review
> 
> > Tools/Scripts/webkitpy/port/xvfbdriver.py:48
> > +        if os.path.exists(port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> > +                xvfb_found = port.host.executive.run_command(port._jhbuild_wrapper + ['which', 'Xvfb'], return_exit_code=True) is 0
> > +        else:
> > +                xvfb_found = port.host.executive.run_command(['which', 'Xvfb'], return_exit_code=True) is 0
> 
> Wouldn't it be easier to simply run Xvfb with the JHBuild wrapper? Doesn't the PATH take care of choosing the correct Xvfb?

I wasn't sure about what would happen if jhbuild is not installed and you try to run Xvfb with the wrapper, so I opted for only doing that when it is installed.

I will modify this if you think it would work ok.

> 
> > Tools/Scripts/webkitpy/port/xvfbdriver.py:58
> > +        if self._port.name().startswith("gtk") and os.path.exists(self._port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> 
> I'd really like to see this move to the port specific file somehow.
> 

The thing is that Xvfb is used by both EFL and GTK, so I don't know how to make this otherwise.

You can't preload the llvmpipe mesa libGL libraries outside of the Xvfb driver or you may break the other drivers (Xorg or Wayland).

> > Tools/Scripts/webkitpy/port/xvfbdriver.py:61
> > +                        error_handler=self._port.host.executive.ignore_error).strip()
> 
> Nit: Missing some spaces around the = sign.
> 

Ok
> > Tools/Scripts/webkitpy/port/xvfbdriver.py:99
> > +        if os.path.exists(self._port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> > +                # Run the Xvfb from the jhbuild
> > +                run_xvfb = self._port._jhbuild_wrapper + run_xvfb
> 
> We don't have any helpers for this?
> 

Helpers for what?

> > Tools/Scripts/webkitpy/port/xvfbdriver.py:107
> > +        if self._llvmpipe_libgl_path:
> > +                # Force the Gallium llvmpipe software rasterizer
> > +                environment['LD_LIBRARY_PATH'] = self._llvmpipe_libgl_path
> > +                # If there were another paths on LD_LIBRARY_PATH append them after llvmpipe_libgl_path
> > +                if os.environ.get('LD_LIBRARY_PATH'):
> > +                        environment['LD_LIBRARY_PATH'] += ':%s' % os.environ.get('LD_LIBRARY_PATH')
> > +
> 
> Can't we set this up in gtk.py? Your indentation is also a bit funky.

I think is a bad idea for the reasons explained above. The preload of mesa should only happen for the Xvfb driver and not for the others.

[Martin Robinson]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 233376 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=233376&action=review


> > Wouldn't it be easier to simply run Xvfb with the JHBuild wrapper? Doesn't the PATH take care of choosing the correct Xvfb?
> 
> I wasn't sure about what would happen if jhbuild is not installed and you try to run Xvfb with the wrapper, so I opted for only doing that when it is installed.
> 
> I will modify this if you think it would work ok.

If you run Xvfb with self._jhbuild_wrapper is seems like you shouldn't need to modify the path at all.

> > > Tools/Scripts/webkitpy/port/xvfbdriver.py:99
> > > +        if os.path.exists(self._port.path_from_webkit_base('WebKitBuild', 'Dependencies')):
> > > +                # Run the Xvfb from the jhbuild
> > > +                run_xvfb = self._port._jhbuild_wrapper + run_xvfb
> > 
> > We don't have any helpers for this?
> > 
> 
> Helpers for what?

Helper for detecting if JHBuild is used. It looks like gtk.py uses the same approach. If we do this more than once it needs to be in a helper called something like "should_use_jhbuild."
> 
> > > Tools/Scripts/webkitpy/port/xvfbdriver.py:107
> > > +        if self._llvmpipe_libgl_path:
> > > +                # Force the Gallium llvmpipe software rasterizer
> > > +                environment['LD_LIBRARY_PATH'] = self._llvmpipe_libgl_path
> > > +                # If there were another paths on LD_LIBRARY_PATH append them after llvmpipe_libgl_path
> > > +                if os.environ.get('LD_LIBRARY_PATH'):
> > > +                        environment['LD_LIBRARY_PATH'] += ':%s' % os.environ.get('LD_LIBRARY_PATH')
> > > +
> > 
> > Can't we set this up in gtk.py? Your indentation is also a bit funky.
> 
> I think is a bad idea for the reasons explained above. The preload of mesa should only happen for the Xvfb driver and not for the others.

It isn't possible to reuse setup_environ_for_server or to create a setup_environ_for_xvfb in gtk.py? I don't think we should be doing GTK+ specific things in this file.

[Carlos Alberto Lopez Perez]

Created attachment 235453 [details]
Patch

[Martin Robinson]

Comment on attachment 235453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235453&action=review

> Tools/gtk/jhbuild.modules:339
> +    <!--- WARNING: At setup_environ_for_server() we check for a directory named "Mesa" when XvfbDriver is used,
> +          so don't change the checkoutdir name even if you update the version  -->

You are using printenv LLVMPIPE_LIBGL_PATH so it seems like this isn't true?

[Carlos Alberto Lopez Perez]

(In reply to comment #6)
> (From update of attachment 235453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235453&action=review
> 
> > Tools/gtk/jhbuild.modules:339
> > +    <!--- WARNING: At setup_environ_for_server() we check for a directory named "Mesa" when XvfbDriver is used,
> > +          so don't change the checkoutdir name even if you update the version  -->
> 
> You are using printenv LLVMPIPE_LIBGL_PATH so it seems like this isn't true?

Right. I will update the comment about this.


Also, I should comment about this point of the previous review:

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 233376 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=233376&action=review
> 
> 
> > > Wouldn't it be easier to simply run Xvfb with the JHBuild wrapper? Doesn't the PATH take care of choosing the correct Xvfb?
> > 
> > I wasn't sure about what would happen if jhbuild is not installed and you try to run Xvfb with the wrapper, so I opted for only doing that when it is installed.
> > 
> > I will modify this if you think it would work ok.
> 
> If you run Xvfb with self._jhbuild_wrapper is seems like you shouldn't need to modify the path at all.
> 

I have tested it, and running any command with jhbuild-wrapper is a no-op if you use the local jhbuild in your environment:

$ Tools/jhbuild/jhbuild-wrapper --gtk run which Xvfb
/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Root/bin/Xvfb

But if you don't use it, that will cause jhbuild to be downloaded and installed on the first run (quite ugly). Check:

$ mv WebKitBuild/Dependencies WebKitBuild/Dependencies.back

$ Tools/jhbuild/jhbuild-wrapper --gtk run which Xvfb
Cloning into 'jhbuild'...
remote: Counting objects: 39410, done.
remote: Compressing objects: 100% (10417/10417), done.
remote: Total 39410 (delta 30410), reused 37509 (delta 28838)
Receiving objects: 100% (39410/39410), 7.97 MiB | 1.78 MiB/s, done.
Resolving deltas: 100% (30410/30410), done.
[....]
Running ./configure --enable-maintainer-mode --prefix=/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Root ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
[....]
 /bin/mkdir -p '/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Root/share/applications'
 /usr/bin/install -c -m 644 jhbuild.desktop '/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Root/share/applications'
make[2]: Leaving directory '/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Source/jhbuild'
make[1]: Leaving directory '/home/clopez/webkit/webkit/WebKitBuild/Dependencies/Source/jhbuild'
/usr/bin/Xvfb

So, IMHO I think is worth to check if _should_use_jhbuild() is true and only execute the commands with the wrapper on that case (that's what the current patch does on xvfbdriver.py)

[Carlos Alberto Lopez Perez]

Created attachment 235492 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 235494 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 236769 [details]
Patch

Rebased patch after r172030

[Martin Robinson]

(In reply to comment #7)

> But if you don't use it, that will cause jhbuild to be downloaded and installed on the first run (quite ugly). Check:

I'm not sure I understand what you mean by "use the local jhbuild in your environment."

[Carlos Alberto Lopez Perez]

(In reply to comment #11)
> (In reply to comment #7)
> 
> > But if you don't use it, that will cause jhbuild to be downloaded and installed on the first run (quite ugly). Check:
> 
> I'm not sure I understand what you mean by "use the local jhbuild in your environment."

I mean, that if you already used the internal jhbuild, then executing "Tools/jhbuild/jhbuild-wrapper --gtk run something" works without problems.

But if you never used the internal jhbuild (aka directory WebKitBuild/Dependencies don't exists), then executing "Tools/jhbuild/jhbuild-wrapper --gtk run something" will cause a checkout of the jhbuild source code and a compilation of jhbuild itself before running "something".

Check what happens in that case at the bottom of the comment c7 https://bugs.webkit.org/show_bug.cgi?id=134070#c7

[Carlos Alberto Lopez Perez]

Comment on attachment 236769 [details]
Patch

Clearing flags on attachment: 236769

Committed r172830: <http://trac.webkit.org/changeset/172830>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.