Bug 131472

Created attachment 229096 [details] Patch

Comment on attachment 229096 [details] Patch LGTM! But bots need to have libxcb-xfixes0-dev installed before landing.

Created attachment 229548 [details] Patch

Created attachment 229674 [details] Patch

Committed r167510: <http://trac.webkit.org/changeset/167510>

Hum I suspect the Debug bot didn't like this change, can you please check what's going on Martin? http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r167519%20(41714)/results.html

Sure. I can take a look, though it will be a day or two before I'll have the opportunity to do it. Feel free to roll out the patch in the meantime. Sorry for the breakage, I didn't keep a close enough eye on the debug bot. I appreciate you pinging the bug.

Comment on attachment 229674 [details] Patch That one landed but I'm going to roll it out for now.

Re-opened since this is blocked by bug 131915

The patch also broke Rego's and my EWS because of missing llvm :)

(In reply to comment #10) > The patch also broke Rego's and my EWS because of missing llvm :) Carlos added LLVM to gtk/jhbuild-optional.modules on r169418 <http://trac.webkit.org/r169418> Should we move LLVM to gtk/jhbuild.modules and retry this patch?

Created attachment 233043 [details] Patch

After discussing with mrobinson, we decide to land this patch to secure pixel test before applying the threaded compositor. So I modified this patch to move LLVM from jhbuild-optional.modules to jhbuild.modules to use LLVM for Mesa as clopez suggested. This modification includes build LLVM with a shared library support for the Mesa, and bumping up the version of Mesa, because 10.0.2 was deprecated.

Now the tests (both layout tests and performance tests) can be run on the native X display instead of the default Xvfb by exporting the environment variable USE_NATIVE_XDISPLAY On a system running other implementation of OpenGL than Mesa (for example, with the Nvidia or Ati/Amd propietary drivers) forcing Mesa when USE_NATIVE_XDISPLAY is set will break. And I believe the same will happen when running the tests on Wayland with proprietary drivers (maybe not a problem now because there are not propietary drivers for wayland AFAIK, but probably in the future, I heard that propietary Nvidia drivers for wayland are coming). To avoid that, I suggest to only force Mesa when the tests are ran with Xvfb. This can be done by checking that none of the environment variables that trigger the other display drivers are set. For example: - if llvmpipe_libgl_path: + if not os.environ.get("WAYLAND_DISPLAY") and not os.environ.get("USE_NATIVE_XDISPLAY") and llvmpipe_libgl_path: Another option (maybe cleaner) is to force mesa directly on the Xvfb driver initialization steps. That way if the other display drivers are selected (Wayland/Weston or Xorg) mesa won't be forced. For example: --- a/Tools/Scripts/webkitpy/port/xvfbdriver.py +++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py @@ -50,6 +50,9 @@ class XvfbDriver(Driver): Driver.__init__(self, *args, **kwargs) self._guard_lock = None self._startup_delay_secs = 1.0 + llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH') + if llvmpipe_libgl_path: + environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', ''))

(In reply to comment #14) > Now the tests (both layout tests and performance tests) can be run on the native X display instead of the default Xvfb by exporting the environment variable USE_NATIVE_XDISPLAY > > On a system running other implementation of OpenGL than Mesa (for example, with the Nvidia or Ati/Amd propietary drivers) forcing Mesa when USE_NATIVE_XDISPLAY is set will break. > > And I believe the same will happen when running the tests on Wayland with proprietary drivers (maybe not a problem now because there are not propietary drivers for wayland AFAIK, but probably in the future, I heard that propietary Nvidia drivers for wayland are coming). Good point. Especially for the performance test, we shouldn't break these drivers. > Another option (maybe cleaner) is to force mesa directly on the Xvfb driver initialization steps. That way if the other display drivers are selected (Wayland/Weston or Xorg) mesa won't be forced. For example: > > --- a/Tools/Scripts/webkitpy/port/xvfbdriver.py > +++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py > @@ -50,6 +50,9 @@ class XvfbDriver(Driver): > Driver.__init__(self, *args, **kwargs) > self._guard_lock = None > self._startup_delay_secs = 1.0 > + llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH') > + if llvmpipe_libgl_path: > + environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', '')) I like this approach, because llvmpipe is used to get constant results of pixel test, it is clear to use llvmpipe only when we are using Xvfb.

Created attachment 233046 [details] Patch

Comment on attachment 233046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233046&action=review > Tools/gtk/jhbuild.modules:342 > + <autotools id="llvm" autogenargs="--enable-optimized --disable-terminfo --disable-zlib --enable-shared"> > + <branch repo="llvm.org" module="trunk" checkoutdir="llvm" version="r206311" > + revision="206311"/> > + </autotools> > + Will the LLVM from the system suffice. We only include dependencies in the main JHBuild if they directly affect the results, otherwise just expect that they are already installed on the system. If possible it should instead be added to Tools/gtk/install-dependencies into the JHBuild section.

(In reply to comment #16) > Created an attachment (id=233046) [details] > Patch --- a/Tools/Scripts/webkitpy/port/xvfbdriver.py +++ b/Tools/Scripts/webkitpy/port/xvfbdriver.py @@ -87,6 +87,11 @@ class XvfbDriver(Driver): server_name = self._port.driver_name() environment = self._port.setup_environ_for_server(server_name) + + llvmpipe_libgl_path = os.environ.get('LLVMPIPE_LIBGL_PATH') + if llvmpipe_libgl_path: + environment['LD_LIBRARY_PATH'] = '%s:%s' % (llvmpipe_libgl_path, os.environ.get('LD_LIBRARY_PATH', '')) + I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa. I suggest moving the code a couple of lines up. Just before: run_xvfb = ["Xvfb", ":%d" % display_id, "-screen", "0", "800x600x%s" % self._xvfb_screen_depth(), "-nolisten", "tcp"] with open(os.devnull, 'w') as devnull: self._xvfb_process = self._port.host.executive.popen(run_xvfb, stderr=devnull)

(In reply to comment #17) > (From update of attachment 233046 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233046&action=review > > > Tools/gtk/jhbuild.modules:342 > > + <autotools id="llvm" autogenargs="--enable-optimized --disable-terminfo --disable-zlib --enable-shared"> > > + <branch repo="llvm.org" module="trunk" checkoutdir="llvm" version="r206311" > > + revision="206311"/> > > + </autotools> > > + > > Will the LLVM from the system suffice. We only include dependencies in the main JHBuild if they directly affect the results, otherwise just expect that they are already installed on the system. If possible it should instead be added to Tools/gtk/install-dependencies into the JHBuild section. The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself. I think it is better to share already existed llvm entry for Mesa.

(In reply to comment #18) > (In reply to comment #16) > > Created an attachment (id=233046) [details] [details] > I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa. > I'm not sure it could be a problem, but changed it to iron out the uncertainty.

Created attachment 233050 [details] Patch

(In reply to comment #19) > The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself. > I think it is better to share already existed llvm entry for Mesa. If it is installed via the optional JHBuild moduleset, Mesa will use it. If we ever ship FTL it will need to work on multiple LLVM versions, at which point we can remove it from the optional JHBuild moduleset. The current way we use the default moduleset is to pull dependencies that don't directly affect test output from the system or the optional moduleset. I don't think this situation differs

(In reply to comment #20) > (In reply to comment #18) > > (In reply to comment #16) > > > Created an attachment (id=233046) [details] [details] [details] > > > I believe that this should be done before executing Xvfb. Otherwise Xvfb would be executed with the system OpenGL libraries, meanwhile the browser would be executed with the OpenGL jhbuild Mesa. We want both (Xvfb and the browser) to be executed with the OpenGL jhbuild Mesa. > > > > I'm not sure it could be a problem, but changed it to iron out the uncertainty. Thanks. I think it could be a problem otherwise, I happen to use the nvidia propietary drivers so I can test this: 1.a) Launch Xfvb with the system libraries (nvidia in my case): $ Xvfb :2 -screen 0 800x600x24 -nolisten tcp 1.b) Test the direct rendering with the Mesa libraries (/usr/lib/mesa-diverted/x86_64-linux-gnu is where Debian "diverts" the libraries from Mesa when you install the nvidia propietary drivers): $ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu DISPLAY=:2 glxinfo | grep "render.*:" Error: couldn't find RGB GLX visual or fbconfig Error: couldn't find RGB GLX visual or fbconfig 1.a) Now launch Xfvb with the mesa libraries: $ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu Xvfb :2 -screen 0 800x600x24 -nolisten tcp 1.b) Test the direct rendering with the Mesa libraries: $ LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu DISPLAY=:2 glxinfo | grep "render.*:" direct rendering: Yes OpenGL renderer string: Gallium 0.4 on llvmpipe (LLVM 3.4, 256 bits)

(In reply to comment #22) > (In reply to comment #19) > > > The FTL needs the specific version of llvm, jhbuild-optional builds llvm itself. > > I think it is better to share already existed llvm entry for Mesa. > > > If it is installed via the optional JHBuild moduleset, Mesa will use it. If we ever ship FTL it will need to work on multiple LLVM versions, at which point we can remove it from the optional JHBuild moduleset. > > The current way we use the default moduleset is to pull dependencies that don't directly affect test output from the system or the optional moduleset. I don't think this situation differs Remember that the original patch r167510 got reverted because the missing LLVM broke the bots and the EWS. If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this. install-dependencies is not ran automatically neither on the bots nor the EWS (that would require super user permissions)

(In reply to comment #24) > If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this. This is always how we handle things. It's a little painful, but it doesn't happen very often. Perhaps we should run install-dependencies automatically.

(In reply to comment #25) > (In reply to comment #24) > > > If we are not going to ship LLVM on the moduleset we should at least inform to the bot and EWS owners to install LLVM before landing this. > > This is always how we handle things. It's a little painful, but it doesn't happen very often. Perhaps we should run install-dependencies automatically. My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing. So, I think we can remove LLVM from the module set after all and just add it in the install-dependencies after FTL has been enabled without specific build of LLVM.

(In reply to comment #26) > My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing. In what way do you think they will be confused or conflict? > So, I think we can remove LLVM from the module set after all and just add it in the install-dependencies after FTL has been enabled without specific build of LLVM. I recommend simply keeping it in the optional moduleset, if possible. I can help out making sure it is installed on all the bots before the patch lands.

Created attachment 233136 [details] Patch

(In reply to comment #27) > (In reply to comment #26) > > > My concern about this is, if a system-installed LLVM and a LLVM from optional modulset exist at the same time, it could be confusing. > > In what way do you think they will be confused or conflict? > There can be a human error to use system's LLVM when try to work on FTL if someone didn't install LLVM from the optional modulset. > > I recommend simply keeping it in the optional moduleset, if possible. I can help out making sure it is installed on all the bots before the patch lands. It is right to use LLVM from the system package at the last, I agree to add the LLVM entry in the install-dependencies under the condition that all developers watch out about which LLVM they are using. I modified the patch to add LLVM in the install-dependencies instead of jhbuild.modules.

Comment on attachment 233136 [details] Patch Let's give this a shot. Thanks!

There are the following errors on the EWS: checking whether gcc supports -Werror=missing-prototypes... yes checking whether gcc supports -fvisibility=hidden... yes checking whether g++ supports -fvisibility=hidden... yes checking whether C compiler accepts -msse4.1... yes checking whether ld supports --gc-sections... yes checking whether to enable assembly... yes, x86_64 checking for dlopen... no checking for dlopen in -ldl... yes checking for dladdr... yes checking for clock_gettime... yes checking for posix_memalign... yes checking for the pthreads library -lpthreads... no checking whether pthreads work without any flags... no checking whether pthreads work with -Kthread... no checking whether pthreads work with -kthread... no checking for the pthreads library -llthread... no checking whether pthreads work with -pthread... yes checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE checking if more special flags are required for pthreads... no checking for PTHREAD_PRIO_INHERIT... no configure: Shared GLAPI should not used with Xlib-GLX, disabling checking for LIBDRM... yes checking for LIBUDEV... no checking for XLIBGL... yes checking for mincore... yes checking for XCB_DRI2... yes checking for llvm-config... /usr/bin/llvm-config checking for RADEON... no configure: error: Package requirements (libdrm_radeon >= 2.4.54) were not met: Requested 'libdrm_radeon >= 2.4.54' but version of libdrm_radeon is 2.4.52 Consider adjusting the PKG_CONFIG_PATH environment variable if you installed software in a non-standard prefix. Alternatively, you may set the environment variables RADEON_CFLAGS and RADEON_LIBS to avoid the need to call pkg-config. See the pkg-config man page for more details. tall-setuid --disable-unit-tests --enable-unix-transport --enable-tcp-transport --enable-local-transport --with-xkb-path=/usr/share/X11/xkb --with-xkb-output=/var/lib/xkb --with-xkb-bin-directory=/usr/bin --enable-introspection Aren't we missing some dependencies?

(In reply to comment #31) > checking for RADEON... no > configure: error: Package requirements (libdrm_radeon >= 2.4.54) were not met: > > Requested 'libdrm_radeon >= 2.4.54' but version of libdrm_radeon is 2.4.52 > > Consider adjusting the PKG_CONFIG_PATH environment variable if you > installed software in a non-standard prefix. Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer.

(In reply to comment #32) > Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer. This configure switch may help: --with-gallium-drivers[=DIRS...] comma delimited Gallium drivers list, e.g. "i915,ilo,nouveau,r300,r600,radeonsi,freedreno,svga,swrast" [default=r300,r600,svga,swrast]

(In reply to comment #33) > (In reply to comment #32) > > Sounds like we want to specifically disable radeon support. We only want to build the software rasterizer. > > This configure switch may help: > > --with-gallium-drivers[=DIRS...] > comma delimited Gallium drivers list, e.g. > "i915,ilo,nouveau,r300,r600,radeonsi,freedreno,svga,swrast" > [default=r300,r600,svga,swrast] Yes, the problem was we didn't set any value to the --with-gallium-drivers= so we got the default value. I'll re-upload the patch adds --with-gallium-drivers=swrast for the Mesa.

Created attachment 233190 [details] Patch

Comment on attachment 233190 [details] Patch Clearing flags on attachment: 233190 Committed r170040: <http://trac.webkit.org/changeset/170040>

All reviewed patches have been landed. Closing bug.