Bug 146740 - [GTK] Build race with -DENABLE_WAYLAND_TARGET=ON
Summary: [GTK] Build race with -DENABLE_WAYLAND_TARGET=ON
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords:
Depends on:
Blocks: 146057
  Show dependency treegraph
 
Reported: 2015-07-08 15:18 PDT by Michael Catanzaro
Modified: 2015-07-13 12:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.09 KB, patch)
2015-07-09 03:47 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-07-08 15:18:09 PDT
In file included from ../../Source/WebCore/platform/graphics/PlatformDisplay.cpp:37:
../../Source/WebCore/platform/graphics/wayland/PlatformDisplayWayland.h:32:10: fatal error: 'WebKitGtkWaylandClientProtocol.h' file not found
#include "WebKitGtkWaylandClientProtocol.h"
         ^
1 error generated.

PlatformDisplayWayland.h needs to be built after WebKitGtkWaylandClientProtocol.h. The problem is, it seems like it should be:

# Wayland protocol extension.
add_custom_command(
    OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.c
    DEPENDS ${WEBCORE_DIR}/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml
    COMMAND wayland-scanner server-header < ${WEBCORE_DIR}/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandServerProtocol.h
    COMMAND wayland-scanner client-header < ${WEBCORE_DIR}/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.h
    COMMAND wayland-scanner code < ${WEBCORE_DIR}/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.c
)

if (ENABLE_WAYLAND_TARGET)
    list(APPEND WebCorePlatformGTK_SOURCES
        platform/graphics/wayland/PlatformDisplayWayland.cpp
        platform/graphics/wayland/WaylandEventSource.cpp
        platform/graphics/wayland/WaylandSurface.cpp

        ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.c
    )

    list(APPEND WebCore_SYSTEM_INCLUDE_DIRECTORIES
        ${WAYLAND_INCLUDE_DIRS}
    )
    list(APPEND WebCore_LIBRARIES
        ${WAYLAND_LIBRARIES}
    )
endif ()

You can see that ${DERIVED_SOURCES_WEBCORE_DIR}/WebKitGtkWaylandClientProtocol.c is listed as the OUTPUT for a command and also a required source for compiling WebCore. I messed around with adding an explicit dependency, but it didn't help. I'm not sure what's wrong. This might be a CMake bug.
Comment 1 Carlos Alberto Lopez Perez 2015-07-08 15:26:04 PDT
I have been bitten by this. Re-running the build makes it continue, but that is not a proper solution.

The build should work on the first try.
Comment 2 Michael Catanzaro 2015-07-08 15:50:23 PDT
In the generated build.ninja file, I see:

#############################################
# Order-only phony target for WebCorePlatformGTK

build cmake_order_depends_target_WebCorePlatformGTK: phony || lib/libbmalloc.a lib/libWTFGTK.a lib/libjavascriptcoregtk-4.0.so lib/libWebCoreGTK.a DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c

Which looks sane. But I also see:

# Custom command for DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c

build DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c: CUSTOM_COMMAND ../../Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml || lib/libWTFGTK.a lib/libWebCoreGTK.a lib/libbmalloc.a lib/libjavascriptcoregtk-4.0.so
  COMMAND = cd /home/mcatanzaro/WebKit/WebKitBuild/GNOME/Source/WebCore && wayland-scanner server-header < /home/mcatanzaro/WebKit/Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > /home/mcatanzaro/WebKit/WebKitBuild/GNOME/DerivedSources/WebCore/WebKitGtkWaylandServerProtocol.h && wayland-scanner client-header < /home/mcatanzaro/WebKit/Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > /home/mcatanzaro/WebKit/WebKitBuild/GNOME/DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.h && wayland-scanner code < /home/mcatanzaro/WebKit/Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml > /home/mcatanzaro/WebKit/WebKitBuild/GNOME/DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c
  DESC = Generating ../../DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c

Which I guess means DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c somehow has a bogus dependency on lib/libWebCoreGTK.a. I confirmed this by running 'ninja -t browse libWebCoreGTK.a' in my build directory, which showed libWebCoreGTK.a depends on lib/libWebCoreGTK.a depends on DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c depends on lib/libWebCoreGTK.a. So if I am reading this right, that is a cyclic dependency; the question is, why does it exist?


DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c
target is built using rule CUSTOM_COMMAND of

../../Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml
lib/libWTFGTK.a (order-only)
lib/libWebCoreGTK.a (order-only)
lib/libbmalloc.a (order-only)
lib/libjavascriptcoregtk-4.0.so (order-only)
dependent edges build:

Source/WebCore/CMakeFiles/WebCorePlatformGTK.dir/__/__/DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c.o
cmake_order_depends_target_WebCorePlatformGTK
Comment 3 Michael Catanzaro 2015-07-08 16:14:34 PDT
Copypasting from [1]:

"""The special rule name phony can be used to create aliases for other targets. For example:

build foo: phony some/file/in/a/faraway/subdir/foo
This makes ninja foo build the longer path. Semantically, the phony rule is equivalent to a plain rule where the command does nothing, but phony rules are handled specially in that they aren’t printed when run, logged (see below), nor do they contribute to the command count printed as part of the build process.

phony can also be used to create dummy targets for files which may not exist at build time. If a phony build statement is written without any dependencies, the target will be considered out of date if it does not exist. Without a phony build statement, Ninja will report an error if the file does not exist and is required by the build."""

This rule:

build DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c: CUSTOM_COMMAND ../../Source/WebCore/platform/graphics/wayland/WebKitGtkWaylandClientProtocol.xml || lib/libWTFGTK.a lib/libWebCoreGTK.a lib/libbmalloc.a lib/libjavascriptcoregtk-4.0.so

Means that lib/libWebCoreGTK.a must be built before DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c if DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c does not already exist. Everything after the || is an order-only dependency, meaning that DerivedSources/WebCore/WebKitGtkWaylandClientProtocol.c does not need to be rebuilt when lib/libWebCoreGTK.a changes. [2]

I can find no justification for why those dependencies exist. I wonder if it's a CMake bug.

[1] https://martine.github.io/ninja/manual.html#_the_literal_phony_literal_rule
[2] https://martine.github.io/ninja/manual.html#ref_dependencies
Comment 4 Michael Catanzaro 2015-07-08 16:32:51 PDT
But it looks like it's normal that these dependencies exist for all WebCore derived sources. E.g.

# Custom command for DerivedSources/WebCore/JSTypeConversions.cpp

build DerivedSources/WebCore/JSTypeConversions.cpp DerivedSources/WebCore/JSTypeConversions.h: CUSTOM_COMMAND ../../Source/WebCore/bindings/scripts/generate-bindings.pl ../../Source/WebCore/bindings/scripts/CodeGenerator.pm ../../Source/WebCore/bindings/scripts/CodeGenerator.pm ../../Source/WebCore/bindings/scripts/CodeGeneratorJS.pm ../../Source/WebCore/bindings/scripts/IDLParser.pm ../../Source/WebCore/bindings/scripts/InFilesParser.pm ../../Source/WebCore/bindings/scripts/preprocessor.pm DerivedSources/WebCore/supplemental_dependency.tmp ../../Source/WebCore/bindings/scripts/IDLAttributes.txt DerivedSources/WebCore/DOMWindowConstructors.idl ../../Source/WebCore/bindings/scripts/CodeGeneratorJS.pm ../../Source/WebCore/testing/TypeConversions.idl || lib/libANGLESupport.a lib/libWTFGTK.a lib/libWebCoreGTK.a lib/libbmalloc.a lib/libjavascriptcoregtk-4.0.so

OK, I don't understand this at all.
Comment 5 Michael Catanzaro 2015-07-08 16:53:19 PDT
My guess is, this is http://www.cmake.org/Bug/view.php?id=15454

Which is fixed in CMake 3.3. So not a WebKit bug, and the workaround is to use make.
Comment 6 Carlos Alberto Lopez Perez 2015-07-08 17:17:52 PDT
Thanks for the investigation.

This information is very useful.

Unfortunately, not only the bots and EWS machines use ninja, but also many developers.

CMake 3.3 isn't even released, so depending on it is not an option. Neither is an option telling developers to not use ninja (or to run the build twice) if they want to use the default build options.

I will try to find a workaround, because this is blocker for bug 146057
Comment 7 Michael Catanzaro 2015-07-08 18:03:44 PDT
(In reply to comment #5)
> My guess is, this is http://www.cmake.org/Bug/view.php?id=15454
> 
> Which is fixed in CMake 3.3. So not a WebKit bug, and the workaround is to
> use make.

That can't be right, because in comment #4 I observed that the dependencies looked just like all our other DerivedSources deps, none of which have this problem. I think Carlos Lopez has found the real problem; I'll let him post about it.
Comment 8 Zan Dobersek 2015-07-08 22:53:46 PDT
I hit this error too, but the PlatformDisplayWayland.h header (which includes WebKitGtkWaylandClientProtocol.h) wasn't included from PlatformDisplay.cpp, but from some other file in a library other than libWebCorePlatformGTK.

The simplest solution is to move the WebKitGtkWaylandClientProtocol.h header inclusion into the PlatformDisplayWayland.cpp file, and use a forward declaration for the wl_webkitgtk struct in the header.

On a side note, PlatformDisplayX11.cpp is built into libWebCore, while PlatformDisplay.cpp and PlatformDisplayWayland.cpp are built into libWebCorePlatformGTK. Would be nice to build these into the same library if possible.
Comment 9 Carlos Alberto Lopez Perez 2015-07-09 03:38:45 PDT
(In reply to comment #8)
> On a side note, PlatformDisplayX11.cpp is built into libWebCore, while
> PlatformDisplay.cpp and PlatformDisplayWayland.cpp are built into
> libWebCorePlatformGTK. Would be nice to build these into the same library if
> possible.

Yes, this is the issue here I think.

Including PlatformDisplayWayland.cpp in libWebCore instead of libWebCorePlatformGTK fixes this bug.

I will upload a patch.
Comment 10 Carlos Alberto Lopez Perez 2015-07-09 03:47:14 PDT
Created attachment 256479 [details]
Patch
Comment 11 Carlos Alberto Lopez Perez 2015-07-13 10:52:52 PDT
reviewers, any comments on this patch? Thanks
Comment 12 Martin Robinson 2015-07-13 10:55:14 PDT
Comment on attachment 256479 [details]
Patch

Sensible.
Comment 13 WebKit Commit Bot 2015-07-13 12:12:53 PDT
Comment on attachment 256479 [details]
Patch

Clearing flags on attachment: 256479

Committed r186768: <http://trac.webkit.org/changeset/186768>
Comment 14 WebKit Commit Bot 2015-07-13 12:12:59 PDT
All reviewed patches have been landed.  Closing bug.