Bug 73559 - Fix OpenGL dependency in CMake build system
Summary: Fix OpenGL dependency in CMake build system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eli Fidler
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-12-01 07:04 PST by Eli Fidler
Modified: 2012-01-11 10:32 PST (History)
11 users (show)

See Also:


Attachments
Patch (5.60 KB, patch)
2011-12-01 07:23 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (5.41 KB, patch)
2011-12-04 08:47 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2011-12-09 08:20 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (4.81 KB, patch)
2011-12-12 15:48 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (3.64 KB, patch)
2012-01-06 14:56 PST, Eli Fidler
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2012-01-11 07:56 PST, Eli Fidler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eli Fidler 2011-12-01 07:04:01 PST
The CMake build system currently checks for desktop OpenGL, but WebKit actually depends on OpenGL ES 2.0.
Comment 1 Eli Fidler 2011-12-01 07:23:43 PST
Created attachment 117416 [details]
Patch
Comment 2 Patrick R. Gansterer 2011-12-01 08:53:27 PST
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)
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-12-01 11:30:20 PST
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.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-12-01 11:31:55 PST
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.
Comment 5 Eli Fidler 2011-12-04 08:47:03 PST
Created attachment 117790 [details]
Patch
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-12-04 17:06:51 PST
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.
Comment 7 Eli Fidler 2011-12-09 08:20:41 PST
Created attachment 118582 [details]
Patch
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-12-09 09:35:36 PST
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.
Comment 9 Nikolas Zimmermann 2011-12-10 01:00:03 PST
Comment on attachment 118582 [details]
Patch

Eli, is this change desired for all cmake based builds? Shouldn't we only do it for us?
Comment 10 Eli Fidler 2011-12-12 12:59:50 PST
It's for all cmake builds. WebGL fundamentally depends on OpenGL ES, not desktop OpenGL.

I'll get the new license up shortly.
Comment 11 Eli Fidler 2011-12-12 15:48:57 PST
Created attachment 118889 [details]
Patch
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-12-12 17:08:51 PST
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.
Comment 13 ChangSeok Oh 2011-12-12 20:22:04 PST
(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.
Comment 14 Eli Fidler 2011-12-13 09:04:37 PST
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.
Comment 15 ChangSeok Oh 2011-12-13 09:32:59 PST
(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.
Comment 16 Eric Seidel (no email) 2011-12-21 11:38:44 PST
Comment on attachment 118889 [details]
Patch

Clearly needs an update, at least for the license if nothing else.
Comment 17 Eli Fidler 2012-01-06 14:56:26 PST
Created attachment 121501 [details]
Patch
Comment 18 Raphael Kubo da Costa (:rakuco) 2012-01-08 14:00:17 PST
So the FindOpenGLES2.cmake file and its associated changes are not needed anymore?
Comment 19 Eli Fidler 2012-01-09 06:09:36 PST
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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-01-09 09:13:48 PST
Alright then, that's even better :)
Comment 21 Daniel Bates 2012-01-09 09:20:54 PST
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.
Comment 22 Eli Fidler 2012-01-11 07:56:25 PST
Created attachment 122022 [details]
Patch
Comment 23 WebKit Review Bot 2012-01-11 10:32:15 PST
Comment on attachment 122022 [details]
Patch

Clearing flags on attachment: 122022

Committed r104722: <http://trac.webkit.org/changeset/104722>
Comment 24 WebKit Review Bot 2012-01-11 10:32:21 PST
All reviewed patches have been landed.  Closing bug.