NEW 229337
GLContextGLX.cpp may sometimes fail build because of X11 include files
https://bugs.webkit.org/show_bug.cgi?id=229337
Summary GLContextGLX.cpp may sometimes fail build because of X11 include files
Daniel Kolesa
Reported Friday, August 20, 2021 4:22:36 PM UTC
The file should probably be marked @no-unify.
Attachments
patch (1.25 KB, patch)
2021-08-20 08:23 PDT, Daniel Kolesa
pnormand: review+
patch (1.18 KB, patch)
2021-10-26 08:15 PDT, Daniel Kolesa
dkolesa: review?
dkolesa: commit-queue?
Daniel Kolesa
Comment 1 Friday, August 20, 2021 4:23:34 PM UTC
Radar WebKit Bug Importer
Comment 2 Friday, August 27, 2021 4:23:21 PM UTC
Daniel Kolesa
Comment 3 Tuesday, October 26, 2021 4:15:23 PM UTC
Created attachment 442489 [details] patch updated patch
Fujii Hironori
Comment 4 Wednesday, October 27, 2021 2:35:45 AM UTC
WebKit has a lot of #undef in WTF and WebCore layer for this issue, for example "#undef True" and "#undef None". Does this change make it possible to remove it? For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68
Fujii Hironori
Comment 5 Wednesday, October 27, 2021 2:39:23 AM UTC
Maybe, No. Because it can't compile if both X11 header and undef-ing headers are included. I think we should take #undef-ing approach.
Fujii Hironori
Comment 6 Wednesday, October 27, 2021 2:57:37 AM UTC
WebKit has ANGLEHeaders.h to include ANGLE headers. https://github.com/WebKit/WebKit/blob/main/Source/WebCore/platform/graphics/angle/ANGLEHeaders.h I think we should add X11Headers.h to include X11 headers. WDYT?
Daniel Kolesa
Comment 7 Wednesday, October 27, 2021 12:48:15 PM UTC
possibly, I marked the file no-unify since in practice other OpenGL files do the same thing (there's a bunch of others in the same file) and I think it's the cleanest way to get around it (it should work unconditionally regardless of what the X11 headers do) but I'm fine either way
Fujii Hironori
Comment 8 Wednesday, October 27, 2021 8:50:53 PM UTC
In my understanding, @no-unify approach can't solve the issue completely because #include order is ruled by WebKit coding style. If a header defining None or True are included after X11 header, it can't compile. Is my understanding right?
Daniel Kolesa
Comment 9 Thursday, October 28, 2021 11:49:15 AM UTC
if the build for the file is non-unified, it solves the issue because the file is compiled on its own - and therefore we get control over *which* headers are included and can make sure they are non-conflicting
Fujii Hironori
Comment 10 Thursday, October 28, 2021 9:04:43 PM UTC
Then, we no longer need the #undef. could you remove all #undef for the issue in the patch?
Daniel Kolesa
Comment 11 Friday, October 29, 2021 1:46:06 PM UTC
you mean for the ANGLE bits?
Fujii Hironori
Comment 12 Friday, October 29, 2021 9:53:17 PM UTC
WebKit has a lot of #undef in WTF and WebCore layer for this issue For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68 If you can solve the issue completely by @no-unify approach, we no longer need the #undef. I think @no-unify approach can't solve the issue completely because #include order is ruled by WebKit coding style. If a header defining None or True are included after X11 header, it can't compile. We need #undef. @no-unify approach isn't a good idea. I think we should add X11Headers.h to include all X11 headers we need.
Daniel Kolesa
Comment 13 Monday, November 1, 2021 2:54:20 AM UTC
(In reply to Fujii Hironori from comment #12) > WebKit has a lot of #undef in WTF and WebCore layer for this issue > For example, > https://github.com/WebKit/WebKit/blob/ > bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68 > If you can solve the issue completely by @no-unify approach, we no longer > need the #undef. > > I think @no-unify approach can't solve the issue completely because > #include order is ruled by WebKit coding style. > If a header defining None or True are included after X11 header, it can't > compile. We need #undef. @no-unify approach isn't a good idea. > I think we should add X11Headers.h to include all X11 headers we need. this is never really a problem for the problematic file in this case, because the X11 headers are included last, and it's a source file - which was my point all along, if you eliminate it being unified with another source file, there should be no problem anymore (your concern would only be a problem if the file itself was a header, or if we didn't control the include order of headers)
Fujii Hironori
Comment 14 Monday, November 1, 2021 3:24:23 AM UTC
Sonny, but I don't understand. Can we remove all #undef? For example, https://github.com/WebKit/WebKit/blob/bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68 If so, please do it. If not, please give up the @no-unify approach, that don't solve the issue.
Daniel Kolesa
Comment 15 Monday, November 1, 2021 5:25:51 PM UTC
(In reply to Fujii Hironori from comment #14) > Sonny, but I don't understand. > Can we remove all #undef? > For example, > https://github.com/WebKit/WebKit/blob/ > bb27304754d0bb686fc3ddfccc42fb8ec1f14d62/Source/WTF/wtf/Threading.h#L64,L68 > If so, please do it. > If not, please give up the @no-unify approach, that don't solve the issue. it does not fix that because that's a header; this patch is specifically for the one failing file
Michael Catanzaro
Comment 16 Friday, January 7, 2022 4:23:56 PM UTC
I agree with Fujii that #undef is preferable to @no-unify. Don't hesitate to use @no-unify if you really need to, but I don't see why #undef couldn't do the job here.
Note You need to log in before you can comment on or make changes to this bug.