WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
patch
(1.18 KB, patch)
2021-10-26 08:15 PDT
,
Daniel Kolesa
dkolesa
: review?
dkolesa
: commit-queue?
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Kolesa
Comment 1
Friday, August 20, 2021 4:23:34 PM UTC
Created
attachment 435989
[details]
patch
Radar WebKit Bug Importer
Comment 2
Friday, August 27, 2021 4:23:21 PM UTC
<
rdar://problem/82439830
>
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.
Top of Page
Format For Printing
XML
Clone This Bug