[GTK] ANGLE's eglplatform.h is build broken with -DENABLE_X11_PLATFORM=OFF

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Looks like ANGLE's eglplatform.h is broken:

In file included from /home/mcatanzaro/buildroot-2019.05/output/build/webkitgtk-2.25.1/Source/ThirdParty/ANGLE/include/EGL/egl.h:39:0,
                 from /home/mcatanzaro/buildroot-2019.05/output/build/webkitgtk-2.25.1/Source/ThirdParty/ANGLE/src/common/PackedEGLEnums_autogen.h:15,
                 from /home/mcatanzaro/buildroot-2019.05/output/build/webkitgtk-2.25.1/Source/ThirdParty/ANGLE/src/common/PackedEGLEnums_autogen.cpp:12:
/home/mcatanzaro/buildroot-2019.05/output/build/webkitgtk-2.25.1/Source/ThirdParty/ANGLE/include/EGL/eglplatform.h:122:10: fatal error: X11/Xlib.h: No such file or directory
 #include <X11/Xlib.h>
          ^~~~~~~~~~~~
compilation terminated.


eglplatform.h has this braindead code:


#elif defined(__unix__) || defined(USE_X11)

/* X11 (tentative)  */
#include <X11/Xlib.h>
#include <X11/Xutil.h>

typedef Display *EGLNativeDisplayType;
typedef Pixmap   EGLNativePixmapType;
typedef Window   EGLNativeWindowType;


which is clearly wrong since defined(__unix__) doesn't imply X11 is available.

I don't know how to be fix because I don't know how eglplatform.h is supposed to work. Defining the EGL types to use either Wayland types or X11 types at compile time seems very strange, considering we have to support both of them at runtime.

[Adrian Perez]

The update to ANGLE done in r245155 has re-imported a version of the
“eglplatform.h” from the upstream sources which is missing the needed
definitions for Wayland. That revision has this part in the diff:

  -#elif defined(WL_EGL_PLATFORM)
  -
  -typedef struct wl_display    *EGLNativeDisplayType;
  -typedef struct wl_egl_pixmap *EGLNativePixmapType;
  -typedef struct wl_egl_window *EGLNativeWindowType;
  -
  -#elif defined(__unix__)
  -
  -#if defined(ANGLE_USE_X11) && !defined(MESA_EGL_NO_X11_HEADERS)
  +#elif defined(__unix__) || defined(USE_X11)

…which is what caused the problem. I will check a bit more thoroughly
the changes introduced in r245155 before making a patch to re-enable
building without X11.

[Michael Catanzaro]

Hm, so this works because we only use GLX and never use EGL at all in the X11 codepath?

[Michael Catanzaro]

Checking the code pre-r245155, I don't see any Wayland-related changes in the changes.diff. Remember when editing ANGLE that your changes will be lost in the next update if you don't also update the changes.diff.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)
> Checking the code pre-r245155, I don't see any Wayland-related changes in
> the changes.diff. Remember when editing ANGLE that your changes will be lost
> in the next update if you don't also update the changes.diff.

In bug #189844 there was a fix for yet one more instance of this issue,
and in that case I did update “changes.diff” — I wonder when did they
get lost afterwards. Anyway, I will make a new patch for this tomorrow.

[Adrian Perez]

Ouch, I ended up dropping the ball on this one. I'll get to it
now because we need this fixed to import 2.26.x into Buildroot
(where non-X11 builds are common).

[Adrian Perez]

Created attachment 379129 [details]
Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

[Adrian Perez]

(In reply to Adrian Perez from comment #6)
> Created attachment 379129 [details]
> Patch

This approach avoids needing to do modifications in ANGLE, which
makes it much less likely to re-introduce this kind of build
issues whenever a new version of ANGLE is imported into the tree.

[Carlos Garcia Campos]

Comment on attachment 379129 [details]
Patch

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

> Source/ThirdParty/ANGLE/PlatformGTK.cmake:8
> +if (ENABLE_WAYLAND_TARGET)
> +    # Explicitly prefer the Wayland platform, otherwise if we are building
> +    # in a system without X11 ANGLE will still try to use the X11 headers.
> +    list(APPEND ANGLE_DEFINITIONS WL_EGL_PLATFORM)
> +elseif (NOT ENABLE_X11_TARGET)

what happens when we build with both x11 and wayland targets enabled?

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #9)
> Comment on attachment 379129 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379129&action=review
> 
> > Source/ThirdParty/ANGLE/PlatformGTK.cmake:8
> > +if (ENABLE_WAYLAND_TARGET)
> > +    # Explicitly prefer the Wayland platform, otherwise if we are building
> > +    # in a system without X11 ANGLE will still try to use the X11 headers.
> > +    list(APPEND ANGLE_DEFINITIONS WL_EGL_PLATFORM)
> > +elseif (NOT ENABLE_X11_TARGET)
> 
> what happens when we build with both x11 and wayland targets enabled?

In that case ANGLE_USE_X11 is defined and the X11 headers are used
by ANGLE's “eglplatform.h” header — that is the current behaviour
already when building with support for both Wayland and X11.

[Carlos Garcia Campos]

Comment on attachment 379129 [details]
Patch

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

>>> Source/ThirdParty/ANGLE/PlatformGTK.cmake:8
>>> +elseif (NOT ENABLE_X11_TARGET)
>> 
>> what happens when we build with both x11 and wayland targets enabled?
> 
> In that case ANGLE_USE_X11 is defined and the X11 headers are used
> by ANGLE's “eglplatform.h” header — that is the current behaviour
> already when building with support for both Wayland and X11.

Please, add a comment here explaining why x11 is not broken even if the wayland define is set unconditionally when building with both targets enabled.

[Adrian Perez]

Created attachment 379444 [details]
Patch


As per a chat with Carlos García, updated to use WL_EGL_PLATFORM if only
Wayland is enabled (and X11 is disabled), plus a comment clarifying why
a Wayland+X11 build works even if only one version of the EGL types is
used.

[Adrian Perez]

Created attachment 379446 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379446 [details]
Patch for landing

Clearing flags on attachment: 379446

Committed r250298: <https://trac.webkit.org/changeset/250298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.