Bug 191692 – REGRESSION(r238016)[GTK][TestWebKitAPI][Ninja] TestJSC can't compile "Bytecodes.h: No such file or directory"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

I don't know why the bots are happy, but I can't build locally anymore. Problem is -IDerivedSources/JavaScriptCore/ is missing from the include path.

[3037/4655] Building CXX object Tools/TestWebKitAPI...TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o
FAILED: Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o 
/usr/lib64/ccache/c++  -DBUILDING_GTK__=1 -DBUILDING_WEBKIT2__ -DBUILDING_WITH_CMAKE=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DGTEST_CREATE_SHARED_LIBRARY=1 -DGTEST_HAS_PTHREAD=1 -DGTEST_HAS_RTTI=0 -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DTEST_WEBKIT2_RESOURCES_DIR=\"/home/mcatanzaro/Projects/WebKit/Tools/TestWebKitAPI/Tests/WebKit\" -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -DWEBKIT_SRC_DIR=\"/home/mcatanzaro/Projects/WebKit\" -IDerivedSources/ForwardingHeaders -IDerivedSources/ForwardingHeaders/JavaScriptCore -IDerivedSources/ForwardingHeaders/JavaScriptCore/glib -IDerivedSources/JavaScriptCore/javascriptcoregtk -I../../Source/WebKit/UIProcess/API/C/soup -I../../Source/WebKit/UIProcess/API/C/gtk -I../../Source/WebKit/UIProcess/API/gtk -I../../Tools/TestWebKitAPI -I. -I../../Source -I../../Source/JavaScriptCore -I../../Source/ThirdParty/gtest/include -I../../Source/WebKit/Platform/IPC -I../../Source/WebKit/Shared -I../../Source/WebKit/Shared/API -I../../Source/WebKit/Shared/API/c -I../../Source/WebKit/Shared/Plugins -I../../Source/WebKit/UIProcess -I../../Source/WebKit/UIProcess/API -I../../Source/WebKit/UIProcess/API/C -I../../Source/WebKit/WebProcess/InjectedBundle -I../../Source/WebKit/WebProcess/InjectedBundle/API/c -I../../Source/bmalloc -IDerivedSources -I../../Source/ThirdParty -isystem ../DependenciesGTK/Root/include/gtk-3.0 -isystem ../DependenciesGTK/Root/include/pango-1.0 -isystem ../DependenciesGTK/Root/include/glib-2.0 -isystem ../DependenciesGTK/Root/lib/glib-2.0/include -isystem ../DependenciesGTK/Root/include/cairo -isystem ../DependenciesGTK/Root/include/pixman-1 -isystem ../DependenciesGTK/Root/include/freetype2 -isystem /usr/include/libpng16 -isystem ../DependenciesGTK/Root/include/libxml2 -isystem /usr/include/libdrm -isystem ../DependenciesGTK/Root/include/harfbuzz -isystem ../DependenciesGTK/Root/include/gdk-pixbuf-2.0 -isystem ../DependenciesGTK/Root/include/gio-unix-2.0 -isystem ../DependenciesGTK/Root/include/atk-1.0 -isystem ../DependenciesGTK/Root/include/at-spi2-atk/2.0 -isystem ../DependenciesGTK/Root/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem /usr/lib64/dbus-1.0/include -isystem ../DependenciesGTK/Root/include/libsoup-2.4 -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align  -fno-strict-aliasing -fno-exceptions -fno-rtti -std=c++14 -gsplit-dwarf -g -fPIE   -Wno-sign-compare -Wno-undef -Wno-unused-parameter -MD -MT Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o -MF Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o.d -o Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o -c ../../Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp
In file included from DerivedSources/ForwardingHeaders/JavaScriptCore/Instruction.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/InstructionStream.h:29,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/UnlinkedCodeBlock.h:34,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/ExecutableBase.h:34,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/ScriptExecutable.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/FunctionExecutable.h:29,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/CallVariant.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/CallEdge.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/PolymorphicCallStubRoutine.h:30,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/CallLinkInfo.h:31,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/CodeBlock.h:35,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/AssemblyHelpers.h:30,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/CCallHelpers.h:30,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/Snippet.h:31,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/DOMJITCallDOMGetterSnippet.h:31,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/DOMJITGetterSetter.h:28,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/Lookup.h:26,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/Lexer.h:25,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/ParseInt.h:29,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/JSBigInt.h:31,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/JSCJSValueInlines.h:33,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/APICast.h:31,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/glib/jsc/JSCContextPrivate.h:22,
                 from ../../Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:24:
DerivedSources/ForwardingHeaders/JavaScriptCore/Opcode.h:32:10: fatal error: Bytecodes.h: No such file or directory
 #include "Bytecodes.h"
          ^~~~~~~~~~~~~
compilation terminated.

[Michael Catanzaro]

Created attachment 354937 [details]
Patch

[Michael Catanzaro]

Dunno about this. It looks like the directory is listed under private include paths for a reason...? Don?

[Alicia Boya García]

Note this error is a flake in the build system. Often after retrying build a couple of times, it works. We'd rather not have to deal with that of course.

Possibly related: https://bugs.webkit.org/show_bug.cgi?id=161477

[Don Olmstead]

I don't think this is the right fix.

WEBKIT_MAKE_FORWARDING_HEADERS(JavaScriptCore
    TARGET_NAME JavaScriptCorePrivateForwardingHeaders
    FILES ${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS}
    DERIVED_SOURCE_DIRECTORIES ${DERIVED_SOURCES_DIR}/JavaScriptCore ${DERIVED_SOURCES_DIR}/JavaScriptCore/inspector
    FLATTENED
)

Is in CMakeLists.txt and it should be copying the contents of that derived sources directory. TestWebKitAPI_DEPENDENCIES has JavaScriptCorePrivateForwardingHeaders as a dependency though so that should be happening. There might be a bug in what Fujii did with the copying of the dependencies.

What happens if you just generate the project and then go into the build directory and do ninja TestJSC?

[Michael Catanzaro]

(In reply to Don Olmstead from comment #4)
> What happens if you just generate the project and then go into the build
> directory and do ninja TestJSC?

$ mkdir -p WebKitBuild/Debug
$ cd WebKitBuild/Debug
$ ../../Tools/jhbuild/jhbuild-wrapper --gtk run cmake -DPORT=GTK -DCMAKE_BUILD_TYPE=Debug -DDEVELOPER_MODE=ON -GNinja ../..
$ ../../Tools/jhbuild/jhbuild-wrapper --gtk run ninja TestJSC

Without my patch, it fails as shown in comment #0.

[Michael Catanzaro]

$ ../../Tools/jhbuild/jhbuild-wrapper --gtk run ninja JavaScriptCorePrivateForwardingHeaders
ninja: no work to do.

So it's getting executed properly.

My guess is that it's perhaps running before Bytecodes.h is generated?

[Michael Catanzaro]

Here's where the header is generated during my build:

[1114/1352] cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/Source/JavaScriptCore && /usr/bin/ruby /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/generator/main.rb --bytecodes_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/Bytecodes.h --init_bytecodes_asm /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/InitBytecodes.asm --bytecode_structs_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/BytecodeStructs.h --bytecode_indices_h /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/JavaScriptCore/BytecodeIndices.h /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/bytecode/BytecodeList.rb

But all of my JSC forwarding headers were already copied before this point.

The last one was copied here:

[1096/1352] cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources && /usr/bin/cmake -E copy_if_different /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/Scripts/make-js-file-arrays.py /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/DerivedSources/ForwardingHeaders/JavaScriptCore/Scripts/make-js-file-arrays.py

Note 1096 < 1114

So it seems to be a dependency issue.

[Don Olmstead]

I have a feeling that it'd be safer if we enumerated the DerivedSources headers that need to be copied so CMake understands all the dependencies. I'll let Fujii chime in there.

[Michael Catanzaro]

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

[Fujii Hironori]

This is the same issue with Bug 182757.
This problem happens if a generated header is used as a forwarding header.

There are two approach.

1. Explicitly list derived headers to copy (Bug 182757 Comment 5)
2. Introduce intermediate target between TestJSC and JavaScriptCore to ensure JavaScriptCore is finished. (Bug 182757 Comment 8)

> +add_custom_target(pre-TestJSC)
> +add_dependencies(TestJSC pre-TestJSC)
> +add_dependencies(pre-TestJSC JavaScriptCore)
 
I'll take a look more deeply today.

[Fujii Hironori]

I'm testing with Ubuntu 18.04.1 LTS.
Here is the command to reproduce.

> rm -rf WebKitBuild/Debug && ./Tools/Scripts/build-webkit --gtk --debug --cmakeargs=-DENABLE_BUBBLEWRAP_SANDBOX=OFF --makeargs=Tools/TestWebKitAPI/CMakeFiles/TestJSC.dir/Tests/JavaScriptCore/glib/TestJSC.cpp.o

This has happened since trunk@238016 (Bug 191439) which adds '#include "Lexer.h"' in ParseInt.h.

[Fujii Hironori]

Created attachment 355023 [details]
Patch

[Michael Catanzaro]

Comment on attachment 355023 [details]
Patch

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

Wow, great investigation. Thanks Fujii.

> Tools/TestWebKitAPI/PlatformGTK.cmake:137
> +# Add an intermediate target between TestJSC and JavaScriptCore to ensure derived headers are copied into the forwarding header directory.
> +add_custom_target(pre-TestJSC)
> +add_dependencies(TestJSC pre-TestJSC)
> +add_dependencies(pre-TestJSC JavaScriptCore)

Hmm. At a minimum, it would need to be done for PlatformWPE.cmake as well.

I'm not sure why the custom target is required. If the JavaScriptCore target is built, then the pre-TestJSC target is immediately satisfied and then TestJSC is ready to run, right? Would it work if we remove the pre-TestJSC target and just use add_dependencies(TestJSC, JavaScriptCore)? Or is that not enough to ensure that the headers are copied?

I fear we're going to have this same problem in other tests, on other ports. Maybe we could instead ensure the target is not linked until after the forwarding headers are copied?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13)
> I fear we're going to have this same problem in other tests, on other ports.

I guess bug #182757 is evidence enough of that.

[Fujii Hironori]

(In reply to Michael Catanzaro from comment #13)
> Hmm. At a minimum, it would need to be done for PlatformWPE.cmake as well.

I will fix it next Monday. Feel free to take over the patch if someone can't wait. 

> I'm not sure why the custom target is required.

It makes compile time dependency.

> If the JavaScriptCore target
> is built, then the pre-TestJSC target is immediately satisfied and then
> TestJSC is ready to run, right?

No. As I mentioned in the ChangeLog entry, it is "link time dependency which means compiling source files of TestJSC starts before the POST_BUILD event of JSC".

> Would it work if we remove the pre-TestJSC
> target and just use add_dependencies(TestJSC, JavaScriptCore)?

I don't think so because TestJSC already has a dependency to JavaScriptCore.

> Or is that
> not enough to ensure that the headers are copied?

It is possible by taking the approach#1.
But, it had a problem for VS generator builds (Bug 182757 Comment 5).
The CMake issue has been addressed(Bug 182757 Comment 36).
I will try the approach#1.
 
> I fear we're going to have this same problem in other tests, on other ports.

Year. The approach#1 is the ideal solution.

> Maybe we could instead ensure the target is not linked until after the
> forwarding headers are copied?

`linked`? This is a compilation error. It is compile time dependency issue.

[Fujii Hironori]

Created attachment 355247 [details]
Patch

[Fujii Hironori]

(In reply to Michael Catanzaro from comment #14)
> (In reply to Michael Catanzaro from comment #13)
> > I fear we're going to have this same problem in other tests, on other ports.
> 
> I guess bug #182757 is evidence enough of that.

JavaScriptCore is the only target which is using WEBKIT_MAKE_FORWARDING_HEADERS and needs to copy derived headers at the moment.
I hope I will fix this issue properly soon.

[Michael Catanzaro]

Thanks!

[WebKit Commit Bot]

Comment on attachment 355247 [details]
Patch

Clearing flags on attachment: 355247

Committed r238374: <https://trac.webkit.org/changeset/238374>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46169534>