Bug 236593

Summary: [GTK][WPE] Inclusion of OpenGLShims.h should not depend on USE(GLX)
Product: WebKit Reporter: Adrian Perez <aperez>
Component: Tools / TestsAssignee: Adrian Perez <aperez>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, clord, contact+bugs.webkit.org, dpa-webkit, Hironori.Fujii, kbr, mcatanzaro, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236592
https://bugs.webkit.org/show_bug.cgi?id=146680
https://bugs.webkit.org/show_bug.cgi?id=208907
Attachments:
Description Flags
Patch ews-feeder: commit-queue-

Description Adrian Perez 2022-02-14 07:52:03 PST
The OpenGLShims.h header does not contain any X11- nor GLX-specific definitions,
and actually in some cases failing to include it can result failing to complete
the build when GLX is not available in the system. This is known to happen in
some setups where the main OpenGL library is provided by libglvnd (see related
bug #236592, too)
Comment 1 Adrian Perez 2022-02-14 07:54:37 PST
Thanks for Haelwenn “lanodan” Monnier who pointed the issue on IRC :)
Comment 2 Adrian Perez 2022-02-14 08:21:07 PST
At some point we may be able to simplify things, see bug #146680 for
an idea to always use libepoxy, but for the build fix I want to solve
here I will do the minimum amount of changes needed, taking into account
that:

 - For USE(LIBEPOXY) we should use <epoxy/gl.h>
 - Else, for USE(OPENGL_ES) we should use <GLES2/gl2.h> directly
 - Otherwise, we use WebCore's "OpenGLShims.h"

(There is also "OpenGLESShims.h", which adds a few definitions only
without trying to piggyback on top of <GLES2/gl2.h>; I think it is
okay to include it only in sources which use definitions from it.)
Comment 3 Michael Catanzaro 2022-02-14 08:44:46 PST
Can we delete this mess if we require epoxy? Seems like past time to do that?
Comment 4 Adrian Perez 2022-02-14 12:52:04 PST
(In reply to Michael Catanzaro from comment #3)
> Can we delete this mess if we require epoxy? Seems like past time to do that?

Yes, that would be bug #146680 which I linked earlier. Here I only want
to do the minimum needed to get things going, because replacing usage
if USE(OPENGL_ES) with USE(LIBEPOXY) requires substantially more work
and testing — but sure I agree it would be neat to remove choices here.
Comment 5 Adrian Perez 2022-02-14 13:56:53 PST
Created attachment 451941 [details]
Patch
Comment 6 Michael Catanzaro 2022-02-14 14:28:55 PST
Comment on attachment 451941 [details]
Patch

This looks suspiciously similar to bug #208907. Please look at that one too before landing.
Comment 7 Adrian Perez 2022-02-15 00:40:13 PST
*** Bug 208907 has been marked as a duplicate of this bug. ***
Comment 8 Adrian Perez 2022-02-15 00:43:48 PST
Let's land this, I have tested builds of WPE with this patch, and of
the GTK port with Mesa, both with and without GLX; and tests passed.
Also lanodan has tried a build with the patch applied in Gentoo with
the set of build options that were making the build fail there, and
that worked fine as well.

The build failure from the api-gtk EWS builder is unrelated to the
patch.
Comment 9 EWS 2022-02-15 01:08:17 PST
Committed r289795 (247260@main): <https://commits.webkit.org/247260@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 451941 [details].
Comment 10 Radar WebKit Bug Importer 2022-02-15 01:09:21 PST
<rdar://problem/88953005>