Fix OpenGL dependency in CMake build system

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eli Fidler]

The CMake build system currently checks for desktop OpenGL, but WebKit actually depends on OpenGL ES 2.0.

[Eli Fidler]

Created attachment 117416 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 117416 [details]
Patch

what about cleaning up the FindOpenGLES2 file? we prefer 4 space indent and a space after "if" and so on (most of the other c++ style guidelines can be applied to CMake too)

[Raphael Kubo da Costa (:rakuco)]

I agree with paroga that the find-file needs some love: it has wrong indentation, a lot of empty lines and checks for some Ogre-specific variables (plus, I don't think we support BORLAND). In the end, the checks should just boil down to:

  find_path(OPENGLES2_INCLUDE_DIRS ... PATHS ... HINTS ...)
  find_library(OPENGLES2_LIBRARIES ... PATHS .. HINTS ...)

  find_path(EGL_INCLUDE_DIRS  ... PATHS ... HINTS ...)
  find_library(EGL_LIBRARIES ... PATHS ... HINTS ...)

  include(FindPackageHandleStandardArgs)
  find_package_handle_standard_args(OPENGLES2 DEFAULT_MSG OPENGLES2_INCLUDE_DIRS OPENGLES2_LIBRARIES)
  find_package_handle_standard_args(EGL DEFAULT_MSG EGL_INCLUDE_DIRS EGL_LIBRARIES)

Note FOO_INCLUDE_DIRS is preferred over FOO_INCLUDE_DIR according to CMake's convention.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 117416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117416&action=review

> Source/cmake/OptionsCommon.cmake:19
> +    FIND_PACKAGE(OpenGLES2 REQUIRED)

You have changed the package being searched, but Source/WebCore/CMakeLists.txt still links to OPENGL_LIBRARIES in the IF (ENABLE_WEBGL) block.

[Eli Fidler]

Created attachment 117790 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 117790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117790&action=review

The find-file is still needlessly complicated, and has some faulty logic in it. I suggest following that structure I mentioned in my previous review.

> Source/cmake/FindOpenGLES2.cmake:18
> +#  EGL_FOUND        - system has EGL
> +#  EGL_INCLUDE_DIRS - the EGL include directory
> +#  EGL_LIBRARIES    - Link these to use EGL

Are there plans to use these EGL variables too? Otherwise they could be dropped.

> Source/cmake/FindOpenGLES2.cmake:22
> +    FIND_LIBRARY (OPENGLES2_gl_LIBRARY libGLESv2)

Shouldn't "libGLESv2" be "GLESv2"?

> Source/cmake/FindOpenGLES2.cmake:27
> +        create_search_paths(/Developer/Platforms)
> +        findpkg_framework(OpenGLES2)
> +        set(OPENGLES2_gl_LIBRARY "-framework OpenGLES")

create_search_paths() and findpkg_framework() seem to be leftovers from the original file which are not defined in webkit.

> Source/cmake/FindOpenGLES2.cmake:60
> +                INCLUDE (FindX11)

Shouldn't this be FIND_PACKAGE(X11)?

> Source/cmake/FindOpenGLES2.cmake:65
> +                IF (NOT APPLE)
> +                    SET (OPENGLES2_LIBRARIES ${X11_LIBRARIES})
> +                ENDIF (NOT APPLE)

We're already inside an ELSE(APPLE) block.

> Source/cmake/FindOpenGLES2.cmake:71
> +IF (OPENGLES2_gl_LIBRARY AND EGL_egl_LIBRARY)

This is always going to fail on WIN32.

[Eli Fidler]

Created attachment 118582 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 118582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118582&action=review

Awesome, this one looks so much better and smaller :)

One non-find-module question now: does this mean WebGL only works with opengl es 2.0 (ie. it had never worked before)?

> Source/cmake/FindOpenGLES2.cmake:4
> +# For details see the accompanying COPYING-CMAKE-SCRIPTS file.

You need to either provide the mentioned COPYING-CMAKE-SCRIPTS or add the license here.

[Nikolas Zimmermann]

Comment on attachment 118582 [details]
Patch

Eli, is this change desired for all cmake based builds? Shouldn't we only do it for us?

[Eli Fidler]

It's for all cmake builds. WebGL fundamentally depends on OpenGL ES, not desktop OpenGL.

I'll get the new license up shortly.

[Eli Fidler]

Created attachment 118889 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 118889 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118889&action=review

> Source/cmake/FindOpenGLES2.cmake:13
> +# 3. The name of the author may not be used to endorse or promote products
> +#    derived from this software without specific prior written permission.

One last nitpick: the WebKit project tends to use the 2-clause BSD license, which does not have this last item. It'd be good if this file could be licensed that way too.

[ChangSeok Oh]

(In reply to comment #10)
> It's for all cmake builds. WebGL fundamentally depends on OpenGL ES, not desktop OpenGL.
Yes. it's true. But OpenGL ES is not suitable on Desktop. So WebKit has porting layer like GraphicsContext3D and ANGLE currently as I know.
Do you want to support embeded device using GLES? If so, I think it's better way adding separated build option to choose opengl backend.

[Eli Fidler]

It's true that some ports use an emulation layer instead of the GLES2.0 headers directly.

The real issue here is that standard desktop OpenGL is never the right choice.

For BlackBerry, we use the OpenGL ES 2.0 headers directly. ANGLE also provides a GLES2/gl2.h. Anything else that wants to build with CMake would have to change the check to find whichever dependencies that port depends on.

I'm happy to just remove the OpenGL check altogether in OptionsCommon.cmake, and leave it up to the port-specific OptionsXXX.cmake, if people prefer that option.

[ChangSeok Oh]

(In reply to comment #14)
> It's true that some ports use an emulation layer instead of the GLES2.0 headers directly.
> 
> The real issue here is that standard desktop OpenGL is never the right choice.
I can't agree. Basically WebKit should run well on Desktop, it means that OpenGL is the best choice for Desktop environment, not OpenGL ES.

> For BlackBerry, we use the OpenGL ES 2.0 headers directly. ANGLE also provides a GLES2/gl2.h. Anything else that wants to build with CMake would have to change the check to find whichever dependencies that port depends on.
> 
> I'm happy to just remove the OpenGL check altogether in OptionsCommon.cmake, and leave it up to the port-specific OptionsXXX.cmake, if people prefer that option.
That's what i suggested. :) we need to add a new build-option to choose gl-backend.
As your patch proposed, you just replace OpenGL to OpenGL ES. It makes some problem on Desktop while checking dependency, because almost desktop environment doesn't have OpenGL ES header and library.

[Eric Seidel (no email)]

Comment on attachment 118889 [details]
Patch

Clearly needs an update, at least for the license if nothing else.

[Eli Fidler]

Created attachment 121501 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

So the FindOpenGLES2.cmake file and its associated changes are not needed anymore?

[Eli Fidler]

It's not needed for BlackBerry, and apparently not for EFL (based on the existing code). If another port needs it, they can add it.

[Raphael Kubo da Costa (:rakuco)]

Alright then, that's even better :)

[Daniel Bates]

Comment on attachment 121501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121501&action=review

> ChangeLog:8
> +        Apparently the EFL port uses desktop OpenGL to implement WebGL, but other ports such as BlackBerry use other libraries like OpenGL ES 2.0 or ANGLE.
> +
> +        Reviewed by NOBODY (OOPS!).

Nit: These lines should be swapped. Also, we wrap lines to around 80 columns in change log entries.

[Eli Fidler]

Created attachment 122022 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122022 [details]
Patch

Clearing flags on attachment: 122022

Committed r104722: <http://trac.webkit.org/changeset/104722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.