Bug 138332 – libwebkit2gtk fails to link without opengl

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Benedikt Morbach]

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

[Zan Dobersek]

Any specific version of WebKitGTK+, or are you using tip-of-tree?

[Benedikt Morbach]

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.

[Alberto Garcia]

It's not just libX11, there's also many other OpenGL symbols in platforms/graphics/*, is OpenGL really optional for us?

[Martin Robinson]

I think you at least need the headers while building.

[Alberto Garcia]

(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?

[Martin Robinson]

(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).

[Alberto Garcia]

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.

[Alberto Garcia]

*** Bug 138347 has been marked as a duplicate of this bug. ***

[Philip Chimento]

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?

[Philip Chimento]

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.

[Philip Chimento]

Created attachment 243920 [details]
Updated patch

Updated patch.

[Philip Chimento]

Created attachment 252286 [details]
Updated patch

Updated patch for 2.8.0.

[WebKit Commit Bot]

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.

[Martin Robinson]

I think this patch is out of date after the latest changes to the CMake build system.

[Michael Catanzaro]

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.

[Martin Robinson]

(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.

[Csaba Osztrogonác]

$ 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

[Philip Chimento]

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

[Philip Chimento]

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.

[Michael Catanzaro]

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.

[Philip Chimento]

Created attachment 252826 [details]
Patch for master

[Philip Chimento]

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.

[Michael Catanzaro]

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.

[Gyuyoung Kim]

(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 !

[Michael Catanzaro]

(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.

[Martin Robinson]

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.

[Gyuyoung Kim]

Comment on attachment 252826 [details]
Patch for master

LGTM, however it looks GTK folks might need to have final review.

[Carlos Garcia Campos]

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

[Philip Chimento]

Created attachment 253122 [details]
Updated patch for master

[Philip Chimento]

(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.

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

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

[Philip Chimento]

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.

[Zan Dobersek]

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.

[Philip Chimento]

Created attachment 255294 [details]
Patch

[Philip Chimento]

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.

[WebKit Commit Bot]

Comment on attachment 255294 [details]
Patch

Clearing flags on attachment: 255294

Committed r185805: <http://trac.webkit.org/changeset/185805>

[Carlos Garcia Campos]

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

[Philip Chimento]

Created attachment 260116 [details]
Patch for 2.8.5

[Philip Chimento]

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.

[Martin Robinson]

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.

[Philip Chimento]

(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.

[Philip Chimento]

Created attachment 260670 [details]
Patch for 2.8 branch

[Michael Catanzaro]

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

[Csaba Osztrogonác]

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).