Bug 138332

Summary: libwebkit2gtk fails to link without opengl
Product: WebKit Reporter: Benedikt Morbach <benedikt.morbach>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, cgarcia, commit-queue, gyuyoung.kim, mcatanzaro, mrobinson, ossy, philip.chimento, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 126492    
Attachments:
Description Flags
Patch
none
Updated patch
none
Updated patch
none
Patch for 2.8.0
none
Patch for master
none
Patch for master
gyuyoung.kim: review+
Updated patch for master
cgarcia: review+, commit-queue: commit-queue-
Patch
none
Patch for 2.8.5
mrobinson: review-
Patch for 2.8 branch none

Description Benedikt Morbach 2014-11-03 16:00:13 PST
If built without OpenGL, libwebkit2gtk fails to link because it is missing '-lX11' and friends.

CMakeFiles/WebKit2.dir/PluginProcess/unix/PluginProcessMainUnix.cpp.o: In function `WebKit::webkitXError(_XDisplay*, XErrorEvent*)':
PluginProcessMainUnix.cpp:(.text+0x11): undefined reference to `XGetErrorText'
CMakeFiles/WebKit2.dir/PluginProcess/unix/PluginProcessMainUnix.cpp.o: In function `WebKit::PluginProcessMain::parseCommandLine(int, char**)':
PluginProcessMainUnix.cpp:(.text._ZN6WebKit17PluginProcessMain16parseCommandLineEiPPc[_ZN6WebKit17PluginProcessMain16parseCommandLineEiPPc]+0xe2): undefined reference to `XSetErrorHandler'
<snip>
...and a lot more errors along those lines

As far as I can tell this is because OptionsGTK.cmake doesn't explicitly call
find_package(X11), but relies on FindOpenGL doing so implicitly.
($X11_*_LIB are referenced in WebCore/PlatformGTK.cmake)

However, OpenGL is an optional dependency and as such can be disabled using
cmakes CMAKE_DISABLE_FIND_PACKAGE_FOO. (see [1]).

I think the correct fix would be to explicitly call find_package(X11), probably inside if (ENABLE_X11_TARGET)

[1] http://www.cmake.org/cmake/help/v3.0/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html
Comment 1 Zan Dobersek 2014-12-02 00:25:10 PST
Any specific version of WebKitGTK+, or are you using tip-of-tree?
Comment 2 Benedikt Morbach 2014-12-02 05:23:43 PST
Tested with 2.6.2 and 2.7.1, but as far as I can tell it's still the same in current tip-of-tree.
Comment 3 Alberto Garcia 2014-12-07 11:29:57 PST
It's not just libX11, there's also many other OpenGL symbols in platforms/graphics/*, is OpenGL really optional for us?
Comment 4 Martin Robinson 2014-12-07 11:35:56 PST
I think you at least need the headers while building.
Comment 5 Alberto Garcia 2014-12-07 12:57:26 PST
(In reply to comment #4)
> I think you at least need the headers while building.

I actually made it work, following these steps:

- Disabling WebGL as well.
- Defining WTF_USE_TEXTURE_MAPPER=1 (but not TEXTURE_MAPPER_GL)
- Building GraphicsLayerTextureMapper.cpp

I can prepare a patch, but do we want this?
Comment 6 Martin Robinson 2014-12-07 13:02:23 PST
(In reply to comment #5)
> (In reply to comment #4)
> > I think you at least need the headers while building.
> 
> I actually made it work, following these steps:
> 
> - Disabling WebGL as well.
> - Defining WTF_USE_TEXTURE_MAPPER=1 (but not TEXTURE_MAPPER_GL)
> - Building GraphicsLayerTextureMapper.cpp
> 
> I can prepare a patch, but do we want this?

I don't think we want to support the fallback TextureMapper configuration (if it even exists any longer).
Comment 7 Alberto Garcia 2014-12-07 13:20:55 PST
Created attachment 242768 [details]
Patch

Here's a quick & dirty patch to make it work, but if we don't want to support
this we should close this and bug 138347 as well.
Comment 8 Alberto Garcia 2014-12-08 01:20:21 PST
*** Bug 138347 has been marked as a duplicate of this bug. ***
Comment 9 Philip Chimento 2015-01-02 20:02:34 PST
I was building 2.6.4 without X11 on OSX (using the Quartz backend of GTK) and ran into a related issue while linking:

Linking CXX shared library ../../lib/libwebkit2gtk-4.0.dylib
Undefined symbols for architecture x86_64:
  "WebCore::GraphicsLayer::create(WebCore::GraphicsLayerFactory*, WebCore::GraphicsLayerClient&)", referenced from:
      WebKit::WebInspectorClient::showPaintRect(WebCore::FloatRect const&) in WebInspectorClient.cpp.o
      WebKit::PageOverlayController::initialize() in PageOverlayController.cpp.o
      WebKit::PageOverlayController::installPageOverlay(WTF::PassRefPtr<WebKit::PageOverlay>, WebKit::PageOverlay::FadeMode) in PageOverlayController.cpp.o
      WebCore::RenderLayerBacking::createGraphicsLayer(WTF::String const&) in libWebCoreGTK.a(RenderLayerBacking.cpp.o)
      WebCore::RenderLayerCompositor::ensureRootLayer() in libWebCoreGTK.a(RenderLayerCompositor.cpp.o)
      WebCore::RenderLayerCompositor::updateOverflowControlsLayers() in libWebCoreGTK.a(RenderLayerCompositor.cpp.o)
ld: symbol(s) not found for architecture x86_64

I think the issue here is that without X11, GraphicsLayerTextureMapper.cpp is the only available implementation of WebCore::GraphicsLayer::create(). It's mentioned in comment 6 that the texture mapper is a fallback; I guess the question is fallback from what? What should provide GraphicsLayer::create() in the case of the Quartz backend WebKitGTK port?
Comment 10 Philip Chimento 2015-01-02 20:26:23 PST
I also had to add "${CMAKE_SOURCE_DIR}/Source/ThirdParty/ANGLE/include/GLSLANG" and "${CMAKE_SOURCE_DIR}/Source/ThirdParty/ANGLE/include/KHR" to the include directories in WebCore/PlatformGTK.cmake, by the way, for this fix to compile. I'll provide an updated patch.
Comment 11 Philip Chimento 2015-01-03 12:37:02 PST
Created attachment 243920 [details]
Updated patch

Updated patch.
Comment 12 Philip Chimento 2015-05-03 15:29:01 PDT
Created attachment 252286 [details]
Updated patch

Updated patch for 2.8.0.
Comment 13 WebKit Commit Bot 2015-05-03 15:31:53 PDT
Attachment 252286 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Martin Robinson 2015-05-03 15:38:25 PDT
I think this patch is out of date after the latest changes to the CMake build system.
Comment 15 Michael Catanzaro 2015-05-03 15:55:59 PDT
The comment says the patch is for 2.8.0. :) It'd be great to have a patch that can apply on master as well.

Also, the ANGLE directories are already appended to WebCore_INCLUDE_DIRECTORIES in WebCore/CMakeLists.txt, so they should be removed from there.
Comment 16 Martin Robinson 2015-05-03 16:21:13 PDT
(In reply to comment #15)
> The comment says the patch is for 2.8.0. :) It'd be great to have a patch
> that can apply on master as well.
> 
> Also, the ANGLE directories are already appended to
> WebCore_INCLUDE_DIRECTORIES in WebCore/CMakeLists.txt, so they should be
> removed from there.

Oops. Sorry about the noise.
Comment 17 Csaba Osztrogon√°c 2015-05-04 08:33:16 PDT
$ Tools/Scripts/svn-apply attachment.cgi\?id=252286
Parsed 0 diffs from patch file(s).

It seems it isn't valid patch for svn-apply, similar to bug126433
Comment 18 Philip Chimento 2015-05-07 22:27:53 PDT
Created attachment 252685 [details]
Patch for 2.8.0

Here's a patch with the requested changes and with a changelog.

If this doesn't apply to master, then I'm not sure when I'll get a chance to prepare a patch that does. Due to the esoteric platform (GTK on OSX) I'm compiling on, it's a rare occurrence that I can compile WebKit to a working state at all. For example, the entire 2.6.x series segfaulted on startup for me, and I never figured out why. On master, it tends to be a losing battle. Now that I have a working state on 2.8.0, I am trying to give you all the patches that I can, before they become stale :-( Any help would be appreciated.

In any case I'll propose this for inclusion on https://trac.webkit.org/wiki/WebKitGTK/2.8.x
Comment 19 Philip Chimento 2015-05-10 11:24:53 PDT
Created attachment 252818 [details]
Patch for master

Here's a patch that applies to master, as well. I've tested it on 2.9.1.
Comment 20 Michael Catanzaro 2015-05-10 12:03:19 PDT
Thanks for these patches.

In WebCore/CMakeLists.txt, I'd prefer to append the two ANGLE include directories once, outside the conditional.

Why is one added in PlatformGTK.cmake? Is that needed only when GRAPHICS_CONTEXT_3D is off? I think that should still be added in WebCore/CMakeLists.txt.
Comment 21 Philip Chimento 2015-05-10 14:50:03 PDT
Created attachment 252826 [details]
Patch for master
Comment 22 Philip Chimento 2015-05-10 14:52:20 PDT
You're welcome.

I initially kept the GLSLANG directory out of CMakeLists.txt because it wasn't added either in the GRAPHICS_CONTEXT_3D case. But it surely can't be harmful to add it anyway, so I just moved it there as you suggested.
Comment 23 Michael Catanzaro 2015-05-11 08:24:22 PDT
EFL bot is having problems:

c++: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.8/README.Bugs> for instructions.

Probably time for them to upgrade GCC, anyway.
Comment 24 Gyuyoung Kim 2015-05-11 20:40:15 PDT
(In reply to comment #23)
> EFL bot is having problems:
> 
> c++: internal compiler error: Killed (program cc1plus)
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <file:///usr/share/doc/gcc-4.8/README.Bugs> for instructions.
> 
> Probably time for them to upgrade GCC, anyway.

Sorry for inconvenient. EFL EWS was sick yesterday. This patch is fine when building locally. Please land it !
Comment 25 Michael Catanzaro 2015-05-12 08:29:05 PDT
(In reply to comment #24) 
> Sorry for inconvenient. EFL EWS was sick yesterday. This patch is fine when
> building locally. Please land it !

Do you have time to review it? :) It looks good to me.
Comment 26 Martin Robinson 2015-05-12 08:37:58 PDT
Comment on attachment 252826 [details]
Patch for master

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

> Source/cmake/OptionsGTK.cmake:281
> +SET_AND_EXPOSE_TO_BUILD(USE_TEXTURE_MAPPER TRUE)
> +

FWIW, we are going to be removing the non-OpenGL version of the texture mapper.
Comment 27 Gyuyoung Kim 2015-05-13 18:14:39 PDT
Comment on attachment 252826 [details]
Patch for master

LGTM, however it looks GTK folks might need to have final review.
Comment 28 Carlos Garcia Campos 2015-05-13 22:54:32 PDT
Comment on attachment 252826 [details]
Patch for master

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

> Source/WebCore/CMakeLists.txt:3120
> +    "${THIRDPARTY_DIR}/ANGLE/include/GLSLANG"

I don't get why you added GLSLANG, if it wasn't needed before, why is it needed now? There's some conflict with the in-tree headers and the installed ones, so I prefer not to change this if it's not needed
Comment 29 Philip Chimento 2015-05-14 08:56:33 PDT
Created attachment 253122 [details]
Updated patch for master
Comment 30 Philip Chimento 2015-05-14 08:58:01 PDT
(In reply to comment #28)
> I don't get why you added GLSLANG, if it wasn't needed before, why is it
> needed now? There's some conflict with the in-tree headers and the installed
> ones, so I prefer not to change this if it's not needed

I tested and indeed it wasn't needed. I think it was needed when I originally wrote this patch on 2.6.x and I just carried it over. I've updated the patch to remove it.
Comment 31 WebKit Commit Bot 2015-05-14 08:59:23 PDT
Attachment 253122 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 WebKit Commit Bot 2015-05-14 11:15:12 PDT
Comment on attachment 253122 [details]
Updated patch for master

Rejecting attachment 253122 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 253122, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 184339 = 76760c75eaef3255b23d23a9f0f0aa94103e47dc
r184340 = 23c2df7821affb0236a545d888809229f80b5dda
r184341 = 5615cb361c49318e67ede4d0f1ecf598beaf7a74
r184344 = fbf429221728313e13303bdcd358c9a4543987e2
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/5510072457232384
Comment 33 Philip Chimento 2015-05-14 22:40:37 PDT
Copying what seems to be the relevant output here:

Processing patch 253122 from bug 138332.
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html

The patch definitely does have a ChangeLog entry, so I'm not sure what's going on here.
Comment 34 Zan Dobersek 2015-05-15 02:35:34 PDT
The changed files in the diff have incorrect paths:
a/Source/WebCore/ChangeLog
a/Source/WebCore/CMakeLists.txt
a/Source/WebCore/platform/graphics/texmap/BitmapTexturePool.cpp
a/Source/cmake/OptionsGTK.cmake
a/ChangeLog

I don't think the diff was generated correctly.
Comment 35 Philip Chimento 2015-06-20 12:04:14 PDT
Created attachment 255294 [details]
Patch
Comment 36 Philip Chimento 2015-06-20 12:06:27 PDT
I regenerated the patch; not sure what wasn't working before. Since the previous patch was already r+, I'm hoping it won't take too long to review this one.
Comment 37 WebKit Commit Bot 2015-06-21 03:07:00 PDT
Comment on attachment 255294 [details]
Patch

Clearing flags on attachment: 255294

Committed r185805: <http://trac.webkit.org/changeset/185805>
Comment 38 Carlos Garcia Campos 2015-07-07 05:53:19 PDT
Comment on attachment 252685 [details]
Patch for 2.8.0

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

> Source/WebCore/PlatformGTK.cmake:26
> +    "${CMAKE_SOURCE_DIR}/Source/ThirdParty/ANGLE/include/GLSLANG"

As in trunk patch I don't get why we need to add GLSLANG.

> Source/WebCore/PlatformGTK.cmake:216
> +    platform/graphics/texmap/GraphicsLayerTextureMapper.cpp
> +

Why do you need to move this file? This file doesn't belong to WebCorePlatformGTK since it doens't use GTK+ at all
Comment 39 Philip Chimento 2015-08-27 20:59:38 PDT
Created attachment 260116 [details]
Patch for 2.8.5
Comment 40 Philip Chimento 2015-08-27 21:01:31 PDT
Here's an updated patch that applies to 2.8.5.

Indeed, I had mistakenly put the file in WebCoreGTK when it should have gone in WebCore.
Comment 41 Martin Robinson 2015-09-04 08:49:11 PDT
Comment on attachment 260116 [details]
Patch for 2.8.5

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

It looks like this patch does more than fix the build when OpenGL is off. Some of these changes look like they are making the TextureMapper unconditional.

> Source/WebCore/CMakeLists.txt:-2985
> -        "${THIRDPARTY_DIR}/ANGLE/"
> -        "${THIRDPARTY_DIR}/ANGLE/include/KHR"

This file is shared between all ports, so I don't think you should remove them from here.

> Source/WebCore/PlatformGTK.cmake:128
> +    platform/graphics/texmap/GraphicsLayerTextureMapper.cpp
> +
>      platform/gtk/ErrorsGtk.cpp

What is the purpose of compiling the TextureMapper unconditionally?

> Source/cmake/OptionsGTK.cmake:355
> +add_definitions(-DWTF_USE_TEXTURE_MAPPER=1)
> +

Ditto.
Comment 42 Philip Chimento 2015-09-04 20:33:13 PDT
(In reply to comment #41)
> Comment on attachment 260116 [details]
> Patch for 2.8.5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260116&action=review
> 
> It looks like this patch does more than fix the build when OpenGL is off.
> Some of these changes look like they are making the TextureMapper
> unconditional.

This is the same as what was committed to master in http://trac.webkit.org/changeset/185805. The TextureMapper (not the TextureMapperGL, though) was already unconditional in practice, because without it there was no implementation of WebCore::GraphicsLayer::create(). I wondered if there was any other way to fix this in comment #9 but I guess there wasn't.

> > Source/WebCore/CMakeLists.txt:-2985
> > -        "${THIRDPARTY_DIR}/ANGLE/"
> > -        "${THIRDPARTY_DIR}/ANGLE/include/KHR"
> 
> This file is shared between all ports, so I don't think you should remove
> them from here.

True, in that case I'll leave them in place here but still add them to PlatformGTK.
Comment 43 Philip Chimento 2015-09-04 20:36:41 PDT
Created attachment 260670 [details]
Patch for 2.8 branch
Comment 44 Michael Catanzaro 2015-09-05 06:41:08 PDT
Comment on attachment 260670 [details]
Patch for 2.8 branch

OK, seems like this is already fixed on master, and the patch is only needed for 2.8. To avoid confusion, I will close this bug as fixed. It's already linked to from [1] which is where we keep the list of backport requests.

Please also note that commit-queue is patches for master.

[1] https://trac.webkit.org/wiki/WebKitGTK/2.8.x
Comment 45 Csaba Osztrogon√°c 2015-09-14 11:02:43 PDT
Comment on attachment 260670 [details]
Patch for 2.8 branch

Cleared review? from attachment 260670 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).