Bug 104760 – [EFL] Allow the build system to find OpenGL ES

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Laszlo Gombos]

Currently cmake/OptionsEfl.cmake requires OpenGL for ENABLE_WEBGL OR WTF_USE_TILED_BACKING_STORE support. This requirement can be relaxed to also enable OpenGL ES and not just OpenGL.

FindOpenGL.cmake is included in CMake distributions but FindOpenGLES2.cmake likely needs to be implemented and checked in into the WebKit tree. Here is one implementation that likely can be used with a permissive license https://bitbucket.org/sinbad/ogre/src/3cbd67467fab3fef44d1b32bc42ccf4fb1ccfdd0/CMake/Packages/FindOpenGLES2.cmake?at=default.

[Yael]

Created attachment 179996 [details]
Patch

[WebKit Review Bot]

Attachment 179996 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/cmake/FindGLES.cmake']" exit_code: 1
Source/cmake/FindGLES.cmake:31:  Use lowercase command "find_path"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:33:  Use lowercase command "find_library"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:35:  Use lowercase command "if"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:36:  No space after "("  [whitespace/parentheses] [5]
Source/cmake/FindGLES.cmake:36:  Use lowercase command "set"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:37:  No space after "("  [whitespace/parentheses] [5]
Source/cmake/FindGLES.cmake:37:  No space before ")"  [whitespace/parentheses] [5]
Source/cmake/FindGLES.cmake:37:  Use lowercase command "set"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:38:  Use lowercase command "endif"  [command/lowercase] [5]
Source/cmake/FindGLES.cmake:40:  Use lowercase command "mark_as_advanced"  [command/lowercase] [5]
Total errors found: 10 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

Created attachment 180005 [details]
Patch

Fix style

[Laszlo Gombos]

Comment on attachment 180005 [details]
Patch

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

> Source/cmake/FindGLES.cmake:8
> +# Copyright (C) 2012 Intel Corporation. All rights reserved.

It seems that you have not used much really from https://bitbucket.org/sinbad/ogre/src/3cbd67467fab3fef44d1b32bc42ccf4fb1ccfdd0/CMake/Packages/FindOpenGLES2.cmake?at=default, so I think this Copyright is ok.

> Source/cmake/FindGLES.cmake:38
> +if (OPENGLES2_gl_LIBRARY)
> +    set(OPENGLES2_LIBRARIES ${OPENGLES2_gl_LIBRARY} ${OPENGLES2_LIBRARIES})
> +    set(OPENGLES2_FOUND "YES")
> +endif (OPENGLES2_gl_LIBRARY and EGL_egl_LIBRARY)

I think we should use the FIND_PACKAGE_HANDLE_STANDARD_ARGS macro instead to align with other FindXX.cmake files.

> Source/cmake/FindGLES.cmake:40
> +mark_as_advanced(OPENGLES2_INCLUDE_DIR OPENGLES2_gl_LIBRARY)

Perhaps we can move this to the FIND_PACKAGE_HANDLE_STANDARD_ARGS macro (and remove it from the other FindXXX.cmake files that are already using FIND_PACKAGE_HANDLE_STANDARD_ARGS macro (e.g. FindFontconfig.cmake). This is unrelated so best to do it in a new bug.

> ChangeLog:11
> +        FindGLES.cmake is loosely based on the example from
> +        https://bitbucket.org/sinbad/ogre/src/3cbd67467fab3fef44d1b32bc42ccf4fb1ccfdd0/CMake/Packages/FindOpenGLES2.cmake?at=default.

I do not think we need to mention this here. The bug has a reference to this if someone wants to extend this simple version in this patch and support more platforms.

[Laszlo Gombos]

(In reply to comment #4)
> (From update of attachment 180005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180005&action=review
> 

> > Source/cmake/FindGLES.cmake:40
> > +mark_as_advanced(OPENGLES2_INCLUDE_DIR OPENGLES2_gl_LIBRARY)
> 
> Perhaps we can move this to the FIND_PACKAGE_HANDLE_STANDARD_ARGS macro (and remove it from the other FindXXX.cmake files that are already using FIND_PACKAGE_HANDLE_STANDARD_ARGS macro (e.g. FindFontconfig.cmake). This is unrelated so best to do it in a new bug.

I just noticed that FIND_PACKAGE_HANDLE_STANDARD_ARGS is a macro that comes with cmake - in that case this part of the patch is correct.

I filed bug 105350 to follow this practice consistently.

[Kalyan]

Comment on attachment 180005 [details]
Patch

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

> Source/cmake/FindGLES.cmake:37
> +    set(OPENGLES2_FOUND "YES")

Do we really need to set this?? I think this is done by cmake already. I am not that familiar with cmake though :) I dont see any other files doing this.

> Source/cmake/FindGLES.cmake:39
> +

Do we need the EGL check here ??

[Kalyan]

(In reply to comment #6)
> (From update of attachment 180005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180005&action=review
 
> > Source/cmake/FindGLES.cmake:39
> > +
> 
> Do we need the EGL check here ??

Meant this part: 

endif (OPENGLES2_gl_LIBRARY and EGL_egl_LIBRARY)

[Yael]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 180005 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=180005&action=review
> 
> > > Source/cmake/FindGLES.cmake:39
> > > +
> > 
> > Do we need the EGL check here ??
> 
> Meant this part: 
> 
> endif (OPENGLES2_gl_LIBRARY and EGL_egl_LIBRARY)
The original file was checking for both EGL and OPENGLES2, my patch is checking only for OPENGLES2, not for EGL.
Is there a reason to keep the EGL checks?

[Yael]

Created attachment 180168 [details]
Patch

Remove the URL from the Changelog, as suggested by comment #4.

[Yael]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 180005 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=180005&action=review
> > 
> > > > Source/cmake/FindGLES.cmake:39
> > > > +
> > > 
> > > Do we need the EGL check here ??
> > 
> > Meant this part: 
> > 
> > endif (OPENGLES2_gl_LIBRARY and EGL_egl_LIBRARY)
> The original file was checking for both EGL and OPENGLES2, my patch is checking only for OPENGLES2, not for EGL.
> Is there a reason to keep the EGL checks?
Sorry, I misunderstood your comment, will fix this.

[Yael]

Created attachment 180191 [details]
Patch

Update based on comments

[Laszlo Gombos]

Comment on attachment 180191 [details]
Patch

lgtm.

[WebKit Review Bot]

Comment on attachment 180191 [details]
Patch

Clearing flags on attachment: 180191

Committed r138217: <http://trac.webkit.org/changeset/138217>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.