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, fujii.hironori, 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-

Adrian Perez
Reported 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)
Attachments
Patch (3.45 KB, patch)
2022-02-14 13:56 PST, Adrian Perez 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adrian Perez
Comment 1 2022-02-14 07:54:37 PST
Thanks for Haelwenn “lanodan” Monnier who pointed the issue on IRC :)
Adrian Perez
Comment 2 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.)
Michael Catanzaro
Comment 3 2022-02-14 08:44:46 PST
Can we delete this mess if we require epoxy? Seems like past time to do that?
Adrian Perez
Comment 4 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.
Adrian Perez
Comment 5 2022-02-14 13:56:53 PST
Created attachment 451941 [details] Patch
Michael Catanzaro
Comment 6 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.
Adrian Perez
Comment 7 2022-02-15 00:40:13 PST
*** Bug 208907 has been marked as a duplicate of this bug. ***
Adrian Perez
Comment 8 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.
EWS
Comment 9 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].
Radar WebKit Bug Importer
Comment 10 2022-02-15 01:09:21 PST
<rdar://problem/88953005>
Note You need to log in before you can comment on or make changes to this bug.