Summary: | [GTK] Load the llvmpipe (Mesa) libraries when running the tests with Xvfb | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||||||||
Component: | Tools / Tests | Assignee: | Carlos Alberto Lopez Perez <clopez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bjonesbe, cgarcia, commit-queue, glenn, gyuyoung.kim, laszlo.gombos, mario, mjs, mrobinson, ossy, pnormand, yoon | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Carlos Alberto Lopez Perez
2014-06-19 12:52:25 PDT
Created attachment 233376 [details]
Patch
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. (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. (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. Created attachment 235453 [details]
Patch
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? (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) Created attachment 235492 [details]
Patch
Created attachment 235494 [details]
Patch
Created attachment 236769 [details] Patch Rebased patch after r172030 (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." (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 Comment on attachment 236769 [details] Patch Clearing flags on attachment: 236769 Committed r172830: <http://trac.webkit.org/changeset/172830> All reviewed patches have been landed. Closing bug. |