Bug 191692

Summary: REGRESSION(r238016)[GTK][TestWebKitAPI][Ninja] TestJSC can't compile "Bytecodes.h: No such file or directory"
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: JavaScriptCoreAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, annulen, bugs-noreply, calvaris, commit-queue, don.olmstead, Hironori.Fujii, jeremyhu, mcatanzaro, pgriffis, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182757
Attachments:
Description Flags
Patch
annulen: review+
Patch
none
Patch none

Description Michael Catanzaro 2018-11-15 08:36:46 PST
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.
Comment 1 Michael Catanzaro 2018-11-15 08:42:43 PST
Created attachment 354937 [details]
Patch
Comment 2 Michael Catanzaro 2018-11-15 08:43:24 PST
Dunno about this. It looks like the directory is listed under private include paths for a reason...? Don?
Comment 3 Alicia Boya GarcĂ­a 2018-11-15 08:48:06 PST
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
Comment 4 Don Olmstead 2018-11-15 09:10:02 PST
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?
Comment 5 Michael Catanzaro 2018-11-15 10:56:29 PST
(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.
Comment 6 Michael Catanzaro 2018-11-15 11:55:25 PST
$ ../../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?
Comment 7 Michael Catanzaro 2018-11-15 12:10:09 PST
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.
Comment 8 Don Olmstead 2018-11-15 12:13:11 PST
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.
Comment 9 Michael Catanzaro 2018-11-15 12:17:37 PST
*** Bug 161477 has been marked as a duplicate of this bug. ***
Comment 10 Fujii Hironori 2018-11-15 18:31:41 PST
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.
Comment 11 Fujii Hironori 2018-11-15 20:04:04 PST
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.
Comment 12 Fujii Hironori 2018-11-15 22:15:38 PST
Created attachment 355023 [details]
Patch
Comment 13 Michael Catanzaro 2018-11-16 07:53:14 PST
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?
Comment 14 Michael Catanzaro 2018-11-16 07:54:42 PST
(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.
Comment 15 Fujii Hironori 2018-11-16 16:26:10 PST
(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.
Comment 16 Fujii Hironori 2018-11-18 19:56:39 PST
Created attachment 355247 [details]
Patch
Comment 17 Fujii Hironori 2018-11-18 19:59:45 PST
(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.
Comment 18 Michael Catanzaro 2018-11-19 08:01:34 PST
Thanks!
Comment 19 WebKit Commit Bot 2018-11-19 08:26:48 PST
Comment on attachment 355247 [details]
Patch

Clearing flags on attachment: 355247

Committed r238374: <https://trac.webkit.org/changeset/238374>
Comment 20 WebKit Commit Bot 2018-11-19 08:26:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-11-19 08:27:32 PST
<rdar://problem/46169534>