Bug 198621 - [GTK] ANGLE's eglplatform.h is build broken with -DENABLE_X11_PLATFORM=OFF
Summary: [GTK] ANGLE's eglplatform.h is build broken with -DENABLE_X11_PLATFORM=OFF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-06 13:14 PDT by Michael Catanzaro
Modified: 2019-09-24 07:58 PDT (History)
15 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2019-09-19 07:02 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2019-09-24 06:29 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (1.81 KB, patch)
2019-09-24 06:42 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-06-06 13:14:55 PDT
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.
Comment 1 Adrian Perez 2019-06-06 13:50:55 PDT
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.
Comment 2 Michael Catanzaro 2019-06-06 13:58:48 PDT
Hm, so this works because we only use GLX and never use EGL at all in the X11 codepath?
Comment 3 Michael Catanzaro 2019-06-06 14:00:29 PDT
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.
Comment 4 Adrian Perez 2019-06-06 16:08:11 PDT
(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.
Comment 5 Adrian Perez 2019-09-18 01:17:31 PDT
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).
Comment 6 Adrian Perez 2019-09-19 07:02:51 PDT
Created attachment 379129 [details]
Patch
Comment 7 Build Bot 2019-09-19 07:03:47 PDT
Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE
Comment 8 Adrian Perez 2019-09-19 07:04:45 PDT
(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.
Comment 9 Carlos Garcia Campos 2019-09-20 00:52:08 PDT
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?
Comment 10 Adrian Perez 2019-09-20 05:14:25 PDT
(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.
Comment 11 Carlos Garcia Campos 2019-09-24 06:20:47 PDT
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.
Comment 12 Adrian Perez 2019-09-24 06:29:47 PDT
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.
Comment 13 Adrian Perez 2019-09-24 06:42:37 PDT
Created attachment 379446 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-09-24 07:58:56 PDT
Comment on attachment 379446 [details]
Patch for landing

Clearing flags on attachment: 379446

Committed r250298: <https://trac.webkit.org/changeset/250298>
Comment 15 WebKit Commit Bot 2019-09-24 07:58:59 PDT
All reviewed patches have been landed.  Closing bug.