Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not include egl.h in public headers #38

Merged
merged 1 commit into from May 8, 2019

Conversation

Projects
None yet
3 participants
@carlosgcampos
Copy link
Contributor

commented May 6, 2019

It includes eglplatform that cand end up including Xlib.h.

Do not include egl.h in public headers
It includes eglplatform that cand end up including Xlib.h.
@carlosgcampos

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

In file included from /usr/include/X11/Xlib.h:44,
                 from /usr/include/EGL/eglplatform.h:124,
                 from /usr/include/EGL/egl.h:39,
                 from /home/cgarcia/gnome/include/wpe-fdo-1.0/wpe/initialize-egl.h:37,
                 from /home/cgarcia/gnome/include/wpe-fdo-1.0/wpe/fdo-egl.h:36,
                 from ../../Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:41,
                 from DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-25.cpp:1:

@carlosgcampos carlosgcampos requested review from zdobersek and aperezdc May 6, 2019

@aperezdc

This comment has been minimized.

Copy link
Member

commented May 6, 2019

If I am understanding correctly, the “problem” here seems to be that the X11 headers get included. That looks to me as a problem in the EGL headers themselves, or something else (maybe the egl.pc file) which causes eglplatform.h to include the X11 headers, when it should be including the Wayland headers.

I think that the solution suggested in the patch is wrong: code should not make guessed about which one is the actual type of EGLDisplay — because it does not have to necessarily be a pointer, and that is why we want to include the header. Could you provide some information about which EGL implementation and architecture are you targeting in your failed build, and how that build is configured?

To give you an example: while EGLDisplay currently is a pointer for Wayland on GNU/Linux, nothing prevents making a Wayland implementation for some other system (let's say OpenBSD) and making EGLDisplay be typedef int EGLDisplay.

@carlosgcampos

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

If I am understanding correctly, the “problem” here seems to be that the X11 headers get included. That looks to me as a problem in the EGL headers themselves, or something else (maybe the egl.pc file) which causes eglplatform.h to include the X11 headers, when it should be including the Wayland headers.

No, that's a common problem with egl. eglplatform.h depends on macros previously defined to decide the native types, and by default X11 is used when non of the conditions are true. We have the same policy in WebKit to not include egl in headers, for the same reason. It's only included in cpp files and defining GBM as a hack, just to ensure that pointer types are used. To ensure wayland is used, we would need to define WL_EGL_PLATFORM in the public header. That could work in this particular case, but it could be problematic, because it's a public header and it's not needed, because we don't really need anything defined in eglplatform.h.

My build failure is only because this header is included in another header defining an enum with 'None' item, and None is defined by XLib.

I think that the solution suggested in the patch is wrong: code should not make guessed about which one is the actual type of EGLDisplay — because it does not have to necessarily be a pointer, and that is why we want to include the header. Could you provide some information about which EGL implementation and architecture are you targeting in your failed build, and how that build is configured?

eglplatform.h defines EGLNativeDisplayType not EGLDisplay. EGLDisplay is defined in egl.h and it's always void* in all platforms AFAIK.

To give you an example: while EGLDisplay currently is a pointer for Wayland on GNU/Linux, nothing prevents making a Wayland implementation for some other system (let's say OpenBSD) and making EGLDisplay be typedef int EGLDisplay.

Again, I think you mean EGLNativeDisplayType, we have always forward declared EGLDisplay as void* in WebKit without any issues.

@aperezdc

This comment has been minimized.

Copy link
Member

commented May 8, 2019

If I am understanding correctly, the “problem” here seems to be that the X11 headers get included. That looks to me as a problem in the EGL headers themselves, or something else (maybe the egl.pc file) which causes eglplatform.h to include the X11 headers, when it should be including the Wayland headers.

No, that's a common problem with egl. eglplatform.h depends on macros previously defined to decide the native types, and by default X11 is used when non of the conditions are true. We have the same policy in WebKit to not include egl in headers, for the same reason. It's only included in cpp files and defining GBM as a hack, just to ensure that pointer types are used. To ensure wayland is used, we would need to define WL_EGL_PLATFORM in the public header. That could work in this particular case, but it could be problematic, because it's a public header and it's not needed, because we don't really need anything defined in eglplatform.h.

I have been checking around for this a bit. The sanctioned way to ensure that the EGL definitions are correct for usage with Wayland is doing the following:

#include <wayland-egl.h>  /* Always BEFORE */
#include <egl.h>

This will handle defining whatever is needed to ensure that only the Wayland headers needed by EGL are included (it does internally some switcheroo with WL_EGL_PLATFORM, but that is an internal detail).

My build failure is only because this header is included in another header defining an enum with 'None' item, and None is defined by XLib.

With the approach suggested above, X11 headers will not be included implicitly.

I think that the solution suggested in the patch is wrong: code should not make guessed about which one is the actual type of EGLDisplay — because it does not have to necessarily be a pointer, and that is why we want to include the header. Could you provide some information about which EGL implementation and architecture are you targeting in your failed build, and how that build is configured?

eglplatform.h defines EGLNativeDisplayType not EGLDisplay. EGLDisplay is defined in egl.h and it's always void* in all platforms AFAIK.

Right, I mixed EGLNativeDisplay with EGLDisplay in my previous messave, but the reasoning applies: user code should not assume anything about the type for any of them.

To give you an example: while EGLDisplay currently is a pointer for Wayland on GNU/Linux, nothing prevents making a Wayland implementation for some other system (let's say OpenBSD) and making EGLDisplay be typedef int EGLDisplay.

Again, I think you mean EGLNativeDisplayType, we have always forward declared EGLDisplay as void* in WebKit without any issues.

Well, that would be because by chance it just happens to be that all the EGL drivers we have targeted so far use some kind of pointer 😉

@carlosgcampos

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

If I am understanding correctly, the “problem” here seems to be that the X11 headers get included. That looks to me as a problem in the EGL headers themselves, or something else (maybe the egl.pc file) which causes eglplatform.h to include the X11 headers, when it should be including the Wayland headers.

No, that's a common problem with egl. eglplatform.h depends on macros previously defined to decide the native types, and by default X11 is used when non of the conditions are true. We have the same policy in WebKit to not include egl in headers, for the same reason. It's only included in cpp files and defining GBM as a hack, just to ensure that pointer types are used. To ensure wayland is used, we would need to define WL_EGL_PLATFORM in the public header. That could work in this particular case, but it could be problematic, because it's a public header and it's not needed, because we don't really need anything defined in eglplatform.h.

I have been checking around for this a bit. The sanctioned way to ensure that the EGL definitions are correct for usage with Wayland is doing the following:

#include <wayland-egl.h>  /* Always BEFORE */
#include <egl.h>

This will handle defining whatever is needed to ensure that only the Wayland headers needed by EGL are included (it does internally some switcheroo with WL_EGL_PLATFORM, but that is an internal detail).

Yes, that's what we should do, but not in headers and even less in public ones. That would be a problem for WebKit. In WebKitGTK we build with support for both wayland and X11 at the same time. If this header is included in a shared header X11 will not work anymore. That's why we have separate cpp files for X11 and Wayland and we only include egl there.

My build failure is only because this header is included in another header defining an enum with 'None' item, and None is defined by XLib.

With the approach suggested above, X11 headers will not be included implicitly.

Yes, even when desired.

I think that the solution suggested in the patch is wrong: code should not make guessed about which one is the actual type of EGLDisplay — because it does not have to necessarily be a pointer, and that is why we want to include the header. Could you provide some information about which EGL implementation and architecture are you targeting in your failed build, and how that build is configured?

eglplatform.h defines EGLNativeDisplayType not EGLDisplay. EGLDisplay is defined in egl.h and it's always void* in all platforms AFAIK.

Right, I mixed EGLNativeDisplay with EGLDisplay in my previous messave, but the reasoning applies: user code should not assume anything about the type for any of them.

EGLDisplay is not platform specific AFAIK, that's why it's defined in egl.h, not in eglplatform.h.

To give you an example: while EGLDisplay currently is a pointer for Wayland on GNU/Linux, nothing prevents making a Wayland implementation for some other system (let's say OpenBSD) and making EGLDisplay be typedef int EGLDisplay.

Again, I think you mean EGLNativeDisplayType, we have always forward declared EGLDisplay as void* in WebKit without any issues.

Well, that would be because by chance it just happens to be that all the EGL drivers we have targeted so far use some kind of pointer

I don't think it's by chance, it's because EGLDisplay is not platform specific.

@carlosgcampos carlosgcampos merged commit c6d3e57 into master May 8, 2019

@carlosgcampos carlosgcampos deleted the cgarcia/egl-h-in-headers branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.