Summary: | GLContextGLX.cpp may sometimes fail build because of X11 include files | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Kolesa <dkolesa> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | Hironori.Fujii, mcatanzaro, pnormand, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Daniel Kolesa
2021-08-20 08:22:36 PDT
Created attachment 435989 [details]
patch
Created attachment 442489 [details]
patch
updated patch
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 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. 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? 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 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? 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 Then, we no longer need the #undef. could you remove all #undef for the issue in the patch? you mean for the ANGLE bits? 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. (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) 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. (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 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. |