Bug 198621

Summary: [GTK] ANGLE's eglplatform.h is build broken with -DENABLE_X11_PLATFORM=OFF
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, aperez, bugs-noreply, cgarcia, commit-queue, dino, don.olmstead, ews-watchlist, graouts, gyuyoung.kim, kondapallykalyan, mcatanzaro, rakuco, ryuan.choi, sergio
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=197676
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Michael Catanzaro
Reported 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.
Attachments
Patch (1.60 KB, patch)
2019-09-19 07:02 PDT, Adrian Perez 		no flags
DetailsFormatted DiffDiff
Patch (1.81 KB, patch)
2019-09-24 06:29 PDT, Adrian Perez 		no flags
DetailsFormatted DiffDiff
Patch for landing (1.81 KB, patch)
2019-09-24 06:42 PDT, Adrian Perez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adrian Perez
Comment 1 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.
Michael Catanzaro
Comment 2 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?
Michael Catanzaro
Comment 3 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.
Adrian Perez
Comment 4 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.
Adrian Perez
Comment 5 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).
Adrian Perez
Comment 6 2019-09-19 07:02:51 PDT
Created attachment 379129 [details] Patch
EWS Watchlist
Comment 7 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
Adrian Perez
Comment 8 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.
Carlos Garcia Campos
Comment 9 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?
Adrian Perez
Comment 10 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.
Carlos Garcia Campos
Comment 11 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.
Adrian Perez
Comment 12 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.
Adrian Perez
Comment 13 2019-09-24 06:42:37 PDT
Created attachment 379446 [details] Patch for landing
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2019-09-24 07:58:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.